[webkit-reviews] review requested: [Bug 227171] Add a new pattern to instruction selector to use EXTR supported by ARM64 : [Attachment 432600] Patch

[bugzilla-daemon at webkit.org]

Robin Morisset <rmorisset at apple.com> has asked	for review:
Bug 227171: Add a new pattern to instruction selector to use EXTR supported by
ARM64
https://bugs.webkit.org/show_bug.cgi?id=227171

Attachment 432600: Patch

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




--- Comment #10 from Robin Morisset <rmorisset at apple.com> ---
Comment on attachment 432600
  --> https://bugs.webkit.org/attachment.cgi?id=432600
Patch

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

I like it overall, but I am concerned that you may have broken the instruction
selection for sbfiz.

> Source/JavaScriptCore/ChangeLog:13
> +	   ### Part A zero extend bitfield ###

I like that, as BitAnd(value, mask) is a lot more canonical throughout the
compiler.
But I just checked, and your pattern for sbfiz looks for the ZShr(Shl(value,
amount), amount) pattern.
Have you tried running testInsertSignedBitfieldInZero64 on ARM64 ? I would
expect it to fail until you replace the pattern for sbfiz in B3LowerToAir.

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1482
> +		       uint64_t mask = width == datasize ? -1ULL : (1ULL <<
width) - 1ULL;

nitpick: instead of `-1ULL` on the true side, I would put
std::numeric_limits<uint64_t>::max(). It is almost certainly equivalent, but
marking a negative number with U(unsigned)LL seems weird to me.