[webkit-reviews] review denied: [Bug 227202] Add ARM64 BIC, ORN, FBXIL to AIR opcode and select them in AIR : [Attachment 432371] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has denied Yijia Huang <yijia_huang at apple.com>'s
request for review:
Bug 227202: Add ARM64 BIC, ORN, FBXIL to AIR opcode and select them in AIR
https://bugs.webkit.org/show_bug.cgi?id=227202

Attachment 432371: Patch

https://bugs.webkit.org/attachment.cgi?id=432371&action=review




--- Comment #12 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 432371
  --> https://bugs.webkit.org/attachment.cgi?id=432371
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=432371&action=review

Didn't you previously make (n >> a) & b the canonical form for extracting a
bitfield?

It's probably worth saying the following though I'm sure you've started to
figure it out: Canonical forms are like pseudoinstructions. Just as phases may
look for instructions, they may look for patterns of instructions, or
instructions with something special about them (like that some operand is a
constant). Also, anytime we can express some logic in B3 IR using existing
instructions, then rather than adding a new instruction, we pick a canonical
form of existing ones. Like BitNot - there is no BitNot opcode. 

When designing canonical forms, we often have many alternative possible ways of
expressing this "pseudoinstruction". So which one do we pick? There are two
things that happen to instructions or instruction patterns:

1. There's a bunch of code to create them.
2. There's a bunch of code to pattern-match them.

If someone wants to write a phase that as part of a transformation needs to
introduce a bitfield extract, then they will want to emit it using canonical
form.

If someone wants to pattern-match the bitfield extract, then they will
pattern-match the canonical form.

Therefore: we should always pick the canonical form that requires the least
amount of code for case (1) and (2), with a preference for less code in (2).

Hence why I think that (n >> a) & b is the natural choice.  Your instruction
selection code illustraces this. To check if (n & a) >> b is a bitfield
extract, you have to count trailing zeroes, then shift down, then check if it's
a mask.  But with (n >> a) & b, you just have to check that a is a mask.

> Source/JavaScriptCore/ChangeLog:36
> +	       Turn this: -value - 1 
> +	       Into this: value ^ -1

This is great.	I can't believe we didn't have this rule already.

Indeed, the canonical form of the BitNot(@x) operation in B3 IR is BitXor(@x,
-1).  So, your patch just reinforces this existing rule.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2851
> +	       // BFXIL Pattern: d = ((n & mask1) >> lsb) | (d & mask2)

I would have made this be (n >> lsb) & mask1

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:3022
> +	       // UBFX Pattern: d = (n & mask) >> lsb

Ditto.