IonMonkey: Infinite loop in global value numbering

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: adrake, Assigned: rpearl)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

7 years ago
Created attachment 548884 [details]
Test case

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

Updated

7 years ago
Blocks: 674689
No longer blocks: 650180
(Assignee)

Comment 1

7 years ago
Created attachment 549289 [details] [diff] [review]
patch v0

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
Assignee: general → rpearl
Status: NEW → ASSIGNED
Attachment #549289 - Flags: review?(adrake)
(Reporter)

Comment 2

7 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

7 years ago
With nits addressed, 
http://hg.mozilla.org/projects/ionmonkey/rev/860aabb7dc93
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Do we want a follow-up bug to remove those copies?
(Assignee)

Comment 5

7 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

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