[webkit-reviews] review granted: [Bug 226984] Add a new pattern to instruction selector to utilize UBFX supported by ARM64 : [Attachment 431625] 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 226984: Add a new pattern to instruction selector to utilize UBFX supported
by ARM64
https://bugs.webkit.org/show_bug.cgi?id=226984

Attachment 431625: Patch

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




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

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

I think it looks right. I just have stylistic comments. The one about using
multiple lines to declare multiple variables is important since that’s the
style we use in JSC (though apparently WebKit style not is ok with what you did
there). 

I do recommend you try to detect if an int has the low 1 bits in a somewhat
more clever way, though the way you did it is right (I think). It’s just there
are these cheap tricks for doing it, like mask & (mask + 1).

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2720
> +		       auto contiguousOnesFromLSB = [&] (int64_t n) -> int64_t
{

I feel like you could be a lot more creative in how you implement this and
probably come up with something that is easier to deal with. 

You want a value that is one less than a power of two. So, if you add one, you
should either get zero (because it was already all 1 bits) or you will get a
value where __builtin_popcount(value) == 1. 

Another technique is mask & (mask + 1). If that is zero then you must have had
the all-1 pattern in the low bits. 

There may be a function in WTF/wtf/MathExtras.h or StdLibExtras.h that does
something like this.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2721
> +			   int64_t mask = 1, width = 1;

I think we would usually use separate lines:

int64_t mask = 1;
int64_t width = 1;