Closed Bug 678629 Opened 13 years ago Closed 13 years ago

IonMonkey: JSOP_BITNOT incorrect in loop conditional in some cases

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: adrake, Assigned: dvander)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Attached 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
Attached patch v0 (obsolete) — Splinter Review
This seems like it might work, but I'll test tomorrow.
Attached 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: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.