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

RESOLVED FIXED in mozilla20

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

Other Branch
mozilla20
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 686863 [details] [diff] [review]
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).
Attachment #686863 - Flags: review?(hv1989)
Attachment #686863 - Flags: review?(hv1989) → review+
Attachment #686863 - Flags: review+
(Assignee)

Updated

6 years ago
Attachment #686863 - Flags: review?(hv1989)
Created attachment 686896 [details]
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...
(Assignee)

Updated

6 years ago
Attachment #686863 - Flags: review?(hv1989)
(Assignee)

Comment 2

6 years ago
Created attachment 686904 [details] [diff] [review]
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.
Assignee: general → bhackett1024
Attachment #686863 - Attachment is obsolete: true
Attachment #686904 - Flags: review?(hv1989)
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
Attachment #686904 - Flags: review?(hv1989) → review+
https://hg.mozilla.org/mozilla-central/rev/743f9704f233
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.