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

Attachment 432059: Patch

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




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

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

This looks good but you're matching rules for BitAnd that won't happen after
reduceStrength does handleCommutativity.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2753
> +		   if (imm(srcValue) || !imm(lsbValue) || lsbValue->asInt() < 0
|| !right->hasInt())

You don't need to exit if srcValue has an imm.	If it had an imm, then it
wouldn't have had ZShr as its opcode.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2770
> +	       if (tryAppendUBFX(left, right) || tryAppendUBFX(right, left))

You can be sure that BitAnd will have its immediate on the right.

I believe we have a reduceStrength rule for that:

	case BitAnd:
	    handleCommutativity();

handleCommutativity() will flip the BitAnd to put the immediate on the right if
it wasn't on the right already.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2829
> +		       if (imm(nValue) || !maskValue->hasInt() || !imm(right)
|| right->asInt() < 0)

No need to bail out of imm() returns true.  If both sides are imm then the
whole thing would have been constant folded and you wouldn't have even seen a
BitAnd.  And if it wasn't constant folded for any reason, then there's no
downside to you matching this rule.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:2846
> +		   return tryAndValueMask(andLeft, andRight) ||
tryAndValueMask(andRight, andLeft);

Ditto - only have to match the case where the immediate is on the right.