[webkit-reviews] review granted: [Bug 204457] Math.max() can yield the wrong result for max(0, -0). : [Attachment 404452] Math.{max, min} fixes, v4

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Xan Lopez <xan.lopez at gmail.com>'s
request for review:
Bug 204457: Math.max() can yield the wrong result for max(0, -0).
https://bugs.webkit.org/show_bug.cgi?id=204457

Attachment 404452: Math.{max,min} fixes, v4

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




--- Comment #22 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 404452
  --> https://bugs.webkit.org/attachment.cgi?id=404452
Math.{max,min} fixes, v4

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

r=me with fixes and if EWS bits are green.

> Source/JavaScriptCore/assembler/testmasm.cpp:2315
> +	       uint64_t result = bitwise_cast<uint64_t>(arg1) &
bitwise_cast<uint64_t>(arg2);
> +	       CHECK_EQ(bitwise_cast<uint64_t>(invoke<double>(andDouble)),
result);

nit: can you rename "result" to "expectedResult"?

> Source/JavaScriptCore/assembler/testmasm.cpp:2335
> +	       uint64_t result = bitwise_cast<uint64_t>(arg1) |
bitwise_cast<uint64_t>(arg2);
> +	       CHECK_EQ(bitwise_cast<uint64_t>(invoke<double>(orDouble)),
result);

Ditto.

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1184
> +		   double result = a < b || (!a && !b && std::signbit(a)) ? a :
(b <= a ? b : a + b);

Please add a comment here to say "The spec for min states that +0 is considered
to be larger than -0."

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1214
> +		   double result = a > b || (!a && !b && !std::signbit(a)) ? a
: (b >= a ? b : a + b);

Please add a comment here to say "The spec for max states that +0 is considered
to be larger than -0."

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6186
> +	   // If both doubles are equal there's a chance one of them is
negative zero, so consider that case.

I think you can replace this comment with "The spec for min and max states that
+0 is considered to be larger than -0." which tells us everything we need to
know about why we need to do this.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6190
> +	   if (node->op() == ArithMax)
> +	       m_jit.andDouble(op1FPR, op2FPR, resultFPR);
> +	   else
> +	       m_jit.orDouble(op1FPR, op2FPR, resultFPR);

Please change this to check for ArithMin instead and flip the 2 cases.	This
will make the code match the FTL case and the other branch above that checks
for ArithMin also.  I think that makes it slightly easier to follow and review
for correctness.

> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:2889
> +	       m_out.branch(

Please also add a comment above here to say "The spec for min and max states
that +0 is considered to be larger than -0."