[webkit-reviews] review granted: [Bug 177281] Add Above/Below comparisons for UInt32 patterns : [Attachment 321629] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 177281: Add Above/Below comparisons for UInt32 patterns
https://bugs.webkit.org/show_bug.cgi?id=177281

Attachment 321629: Patch

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




--- Comment #18 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 321629
  --> https://bugs.webkit.org/attachment.cgi?id=321629
Patch

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

r=me
I'm not entirely convinced we want all this machinery for a very specific type
of program written in a very specific way at the JS source level. That said, I
don't want to keep you from landing this.

I still wonder if a better approach would've been to teach OSR exit in a way
similar to it undoing math operations that we eliminate Uint32ToNumber and just
perform the operation upon exit.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2005
> +		   static_cast<BinaryOpNode*>(left)->m_notCastUnsigned = true;

I'd assert here it's urshift

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2007
> +		   static_cast<BinaryOpNode*>(right)->m_notCastUnsigned = true;

ditto

> Source/JavaScriptCore/dfg/DFGIntegerRangeOptimizationPhase.cpp:1143
> +			   // FIXME: Handle CompareBelow and CompareBelowEq.

Please remove or link to a bug.

> Source/JavaScriptCore/parser/Nodes.h:1084
> +	   bool m_notCastUnsigned { false };

Name nit: I think I'd flip the boolean state of this variable and maybe name it
as:
bool m_shouldToUnsignedResult { true }
and the pattern matching code can set it to false
I think that'll be more clear from reading the code