Bug 227203 – Add ARM64 SBFX and SBFIZ to instruction selector

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Yijia Huang]

...

[Yijia Huang]

Created attachment 432454 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/79899430>

[Yijia Huang]

Created attachment 432485 [details]
Patch

[Sam Weinig]

Comment on attachment 432485 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +            1. Itroduce a strength reduction rule for sign extending bitfield.
> +            2. Add Signed Bitfield Extract (SBFX) and Signed Bitfield Insert 
> +               in Zero (SBFIZ) to Air opcode to serve intruciton selector.

Two small typos here:

Itroduce -> Introduce
intruciton -> instruction

> Source/JavaScriptCore/ChangeLog:16
> +        According to Bit Twiddling Hacks, there are two ways to sign extends bitfield.

Typo: 

sign extends bitfield -> sign extend bitfields

[Filip Pizlo]

Comment on attachment 432485 [details]
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.

[Yijia Huang]

(In reply to Sam Weinig from comment #4)
> Comment on attachment 432485 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432485&action=review
> 
> > Source/JavaScriptCore/ChangeLog:11
> > +            1. Itroduce a strength reduction rule for sign extending bitfield.
> > +            2. Add Signed Bitfield Extract (SBFX) and Signed Bitfield Insert 
> > +               in Zero (SBFIZ) to Air opcode to serve intruciton selector.
> 
> Two small typos here:
> 
> Itroduce -> Introduce
> intruciton -> instruction
> 
> > Source/JavaScriptCore/ChangeLog:16
> > +        According to Bit Twiddling Hacks, there are two ways to sign extends bitfield.
> 
> Typo: 
> 
> sign extends bitfield -> sign extend bitfields

Thanks for the review.

[Yijia Huang]

(In reply to Filip Pizlo from comment #5)
> Comment on attachment 432485 [details]
> 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.

Thanks for the review.

[Yijia Huang]

Created attachment 432500 [details]
Patch

[EWS]

Committed r279378 (239243@main): <https://commits.webkit.org/239243@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 432500 [details].