IonMonkey: JSOP_BITNOT incorrect in loop conditional in some cases

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: adrake, Assigned: dvander)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

8 years ago
Posted file Test case
Attached test case prints 1 on ionmonkey x86 debug builds, and should print 0.
The bug here is that phis are not being placed properly. Investigating.
This bug is a whole thing. The way we deal with breaks and continues is wrong, because the exit definition of a break block does not get replaced if its definition is replaced with a phi at the loop header.

The easiest way to fix this, I think, is to change blocks to take an exit snapshot, so exit definitions are implicitly part of use chains. No matter what we also have to change the order in which the catch-all break block is created.
Assignee: general → dvander
Status: NEW → ASSIGNED
Posted patch v0 (obsolete) — Splinter Review
This seems like it might work, but I'll test tomorrow.
Posted patch fixSplinter Review
The final fix is to refactor loop handling into two cases:

(1) "Broken" loops, or loops that are known to never iterate because the backedge is dominated by a break or return. In this case if there is a successor (either an explicit one or one implicitly created by breaks), we resume parsing from there. Otherwise we escape out.

(2) Actual loops, where if there are successors (explicit or breaks) we need to propagate phi placement down to each one. Then we can create the break catch-block if needed.
Attachment #555029 - Attachment is obsolete: true
Attachment #555266 - Flags: review?(sstangl)
Comment on attachment 555266 [details] [diff] [review]
fix

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

Everything looks good. Efforts to break it failed.

::: js/src/ion/MIRGraph.h
@@ +180,5 @@
>      // Sets a back edge. This places phi nodes and rewrites instructions within
> +    // the current loop as necessary.
> +    bool setBackedge(MBasicBlock *block);
> +
> +    // Propagates phis placed in a loop header down to their corresponding

"down to successor blocks"?
Attachment #555266 - Flags: review?(sstangl) → review+
http://hg.mozilla.org/projects/ionmonkey/rev/7563960aa7ff
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.