Closed
Bug 674656
Opened 13 years ago
Closed 13 years ago
IonMonkey: Infinite loop in global value numbering
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: adrake, Assigned: rpearl)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
300 bytes,
application/javascript
|
Details | |
5.15 KB,
patch
|
adrake
:
review+
|
Details | Diff | Splinter Review |
Attached test case loops forever in global value numbering phase on x86 debug builds.
Reporter | ||
Updated•13 years ago
|
Assignee | ||
Comment 1•13 years ago
|
||
Attached is a patch. The issue is that we cannot assume that phi nodes are congruent if they are in different blocks, since we don't know which predecessor control flow came from. Consider: B1: x1 = 1 y1 = 2 if (x1 < y1) goto b3 goto b4 B2: x2 = 2 y2 = 1 if (x2 < y2) goto b4 goto b3 B3: x3 = phi(x1, x2) ... B4: x4 = phi(x1, x2) ... In both cases we have the same phi nodes, but if control reaches B3, the value is distinct from if control reaches B4. My patch adds a lot of extra GVN spew and an additional assert or two. The heart of the patch is the just the additional code in MIR.cpp
Reporter | ||
Comment 2•13 years ago
|
||
Comment on attachment 549289 [details] [diff] [review] patch v0 Review of attachment 549289 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/ion/MIRGraph.h @@ -413,5 @@ > { } > > MDefinitionIterator operator ++(int) { > MDefinitionIterator old(this); > - if (more()) I think this is necessary in some loop end conditions. ::: js/src/jit-test/tests/ion/bug674656.js @@ +14,5 @@ > + } > + } while (p0); > + } > +} > +f0(0); Nit: Comment on success condition. (/* Don't assert */ is fine)
Attachment #549289 -
Flags: review?(adrake) → review+
Assignee | ||
Comment 3•13 years ago
|
||
With nits addressed, http://hg.mozilla.org/projects/ionmonkey/rev/860aabb7dc93
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Do we want a follow-up bug to remove those copies?
Assignee | ||
Comment 5•13 years ago
|
||
(In reply to comment #4) > Do we want a follow-up bug to remove those copies? Yeah, probably a good idea. I already have a patch which uses MDefinitionIterator and adds a 'removeDefAt' function. I can split out those changes to use in said bug, and fix bug 671430 in the process too.
Assignee | ||
Comment 6•13 years ago
|
||
(In reply to comment #5) > Yeah, probably a good idea. I already have a patch which uses > MDefinitionIterator and adds a 'removeDefAt' function. I can split out those > changes to use in said bug, and fix bug 671430 in the process too. Bug 675244: copy instructions are not removed from phi nodes.
You need to log in
before you can comment on or make changes to this bug.
Description
•