816786 - IonMonkey: don't rely on sparse resume points when eliding negative zero checks

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

Currently, in an operation x*y + z the negative zero check can be removed on x*y only if there is no resume point capturing the result of x*y --- this means that any later bailouts will restart before the x*y and produce the -0 value.  With bug 814997 there will always be a resume point after x*y, so the negative zero check can never be removed.  It's not good to just skip insertion of this resume point, as having it precede the x*y means that both x and y must be kept alive after the multiply, at which point they may actually be dead.

It would be better if removal of the negative zero checks was independent from the placement of resume points.  The attached patch restructures how these checks are eliminated when used in adds, and in a smart enough way to not require any new negative zero checks in v8-crypto's am3 (where these checks really hurt).

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: Testcase going bad

Thought a little bit more about this and we can't remove the "rhs". This is the same problem it use to have. We can't eagerly remove a check without checking that the other operand definitely can't be negative zero, anymore!

Testcase is the same version already in the tree, but now with the operands of the addition switched...

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch

Good catch, duh, of course there is no guarantee about the order of execution of lhs vs. rhs with SSA.  This is pretty easy to address though, instead of reasoning about lhs/rhs just reason about first vs. second executed operands.  Since both operands dominate the addition one must dominate the other.

Comment 3

[Hannes Verschore [:h4writer]]

Comment on attachment 686904 [details] [diff] [review]
patch

Review of attachment 686904 [details] [diff] [review]:
-----------------------------------------------------------------

1) Can you add the testcase too?
2) Can you put a comment in EdgeCaseAnalysis.h (or even Ion.cpp) that after this pass, the operations may not get reordered anymore after this pass

::: js/src/ion/EdgeCaseAnalysis.cpp
@@ +38,2 @@
>      }
>  

When I was an intern and when IM was really in an early stage I think I've wanted to use the numbers for an optimization or something and was told it was slow or something. Never really understood why. Now I can't see a problem using this...

::: js/src/ion/MIR.cpp
@@ +610,4 @@
>              }
>  
> +            if (def == first) {
> +                // Negative zero checks can be removed on the first operand

*first executed operand

@@ +610,5 @@
>              }
>  
> +            if (def == first) {
> +                // Negative zero checks can be removed on the first operand
> +                // only if it is guaranteed the second operand will produce a

*second executed operand

Comment 4

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/743f9704f233

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/743f9704f233