Attached test case loops forever in global value numbering phase on x86 debug builds.

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

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)

With nits addressed, http://hg.mozilla.org/projects/ionmonkey/rev/860aabb7dc93

Do we want a follow-up bug to remove those copies?

(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.

(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.