[webkit-reviews] review granted: [Bug 227203] Add ARM64 SBFX and SBFIZ to instruction selector : [Attachment 432485] Patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Yijia Huang
<yijia_huang at apple.com>'s request for review:
Bug 227203: Add ARM64 SBFX and SBFIZ to instruction selector
https://bugs.webkit.org/show_bug.cgi?id=227203

Attachment 432485: Patch

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




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

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

> Source/JavaScriptCore/ChangeLog:35
> +	       Turn this: ((bf & mask1) ^ mask2) - mask2 
> +	       Into this: (bf << amount) >> amount

Wow, this is super cool.

You should at some point also add a reduction for bitfield zero-extend	I mean,
bitfield zero-extend is a funny way of saying "x & mask" where mask is !(mask &
(mask + 1)).  You can also say it by doing "(x << amount) >> amount", but
that's not the canonical form.	Totally separate from this patch though.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:3036
> +		   if (left->opcode() != SShr || !canBeInternal(left))

A lot of the canBeInternal calls don't pass my sniff test.  It's not important
for you to change this in this patch, but we should reconsider all of these
eventually.

Let's just take this one as an example.

Say you have a program that does the following (capital letters are constants):

a = x << C
b = a >> C
c = b << D

print(a)
print(b)
print(c)

Clearly, the pattern won't match because of !canBeInternal for a and b.  So,
this would emit three separate instructions.  But what if we removed
canBeInternal?	It would still be just three separate instructions, and they
wouldn't be any more expensive.  Now imagine if we remove the print(b), above. 
Then, with the canBeInternal check, we're emitting three instructions.	Without
the canBeInternal check, we'd emit only two (x << C and SBFIZ to compute c). 
And that would be less expensive.

Basically, to decide if canBeInternal is profitable or not, you want to imagine
a program where every internal value is actually used elsewhere, and then ask
yourself: would the code I generate with my pattern be worse than without my
pattern for that code if I never checked canBeInternal?  If the answer is that
the pattern doesn't cause worst code generation in the
all-internals-are-captured case, then you don't want canBeInternal checks.