[webkit-reviews] review granted: [Bug 227138] Add a new pattern to B3ReduceStrength based on Bug 226984 : [Attachment 431779] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jun 18 10:16:35 PDT 2021


Filip Pizlo <fpizlo at apple.com> has granted Yijia Huang
<yijia_huang at apple.com>'s request for review:
Bug 227138: Add a new pattern to B3ReduceStrength based on Bug 226984
https://bugs.webkit.org/show_bug.cgi?id=227138

Attachment 431779: Patch

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




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

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

I think this is correct, but I'd make the two small changes I suggest.

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1044
> +		   && !(m_value->child(0)->child(0)->hasInt())

I don't think you need this condition.	Your transformation is still correct
even if that value is a constant.

If it was a constant then it would probably also get constant-folded, but you
don't have to worry about that.  It's better to just have only the checks you
really need.

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:1048
> +		   int64_t const1 = m_value->child(0)->child(1)->asInt();
> +		   uint64_t const2 = m_value->child(1)->asInt();

Can we give these better names?  Like, I'd use shiftAmount instead of const1. 
And I'd use mask for const2.


More information about the webkit-reviews mailing list