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

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has denied 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 403678: Math.{max,min} fixes, v3

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




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

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

r- for now because I think this patch can use a bit more work.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6188
> +	   MacroAssembler::Jump opEqual =
m_jit.branchDouble(MacroAssembler::DoubleEqualAndOrdered, op1FPR, op2FPR);
> +
>	   MacroAssembler::Jump op1Less = m_jit.branchDouble(node->op() ==
ArithMin ? MacroAssembler::DoubleLessThanAndOrdered :
MacroAssembler::DoubleGreaterThanAndOrdered, op1FPR, op2FPR);
>  
> -	   // op2 is eather the lesser one or one of then is NaN
> -	   MacroAssembler::Jump op2Less = m_jit.branchDouble(node->op() ==
ArithMin ? MacroAssembler::DoubleGreaterThanOrEqualAndOrdered :
MacroAssembler::DoubleLessThanOrEqualAndOrdered, op1FPR, op2FPR);
> +	   // op2 is either the lesser one or one of then is NaN
> +	   MacroAssembler::Jump op2Less = m_jit.branchDouble(node->op() ==
ArithMin ? MacroAssembler::DoubleGreaterThanAndOrdered :
MacroAssembler::DoubleLessThanAndOrdered, op1FPR, op2FPR);

Can we implement this code as follows instead?

    MacroAssembler::Jump op1Less = m_jit.branchDouble(node->op() == ArithMin ?
MacroAssembler::DoubleLessThanAndOrdered :
MacroAssembler::DoubleGreaterThanAndOrdered, op1FPR, op2FPR);
    MacroAssembler::Jump opNotEqual =
m_jit.branchDouble(MacroAssembler::DoubleNotEqualOrUnordered, op1FPR, op2FPR);

    // If both doubles are equal there's a chance one of them is negative zero,
so consider that case.
    if (node->op() == ArithMax)
	m_jit.andDouble(op1FPR, op2FPR, resultFPR);
    else
	m_jit.orDouble(op1FPR, op2FPR, resultFPR);
    done.append(m_jit.jump());

    opNotEqual.link(&m_jit);

    // op2 is either the lesser one or one of then is NaN.
    MacroAssembler::Jump op2Less = m_jit.branchDouble(node->op() == ArithMin ?
MacroAssembler::DoubleGreaterThanAndOrdered :
MacroAssembler::DoubleLessThanAndOrdered, op1FPR, op2FPR);

This way the a < b (for min) or a > b (for max) case still take the same number
of instructions.
Only the >= (for min) or <= (for max) case incurs the extra == check.

If I were to guess, I would guess that a almost never == b.  So, optimistically
checking for a < b or a > b first would be a win half of the time.

Please do the same for the FTL code below.

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:6214
> +	   done.append(m_jit.jump());
> +
> +	   // If both doubles are equal there's a chance one of them is
negative zero, so consider that case.
> +	   opEqual.link(&m_jit);
> +	   if (node->op() == ArithMax)
> +	       m_jit.andDouble(op1FPR, op2FPR, resultFPR);
> +	   else
> +	       m_jit.orDouble(op1FPR, op2FPR, resultFPR);
> +

Remove this.