Page MenuHomePhabricator

NewGVN: Fix PR 31686 by rewriting store leader handling.
ClosedPublic

Authored by dberlin on Jan 20 2017, 1:33 AM.

Details

Reviewers
davide
Summary

This rewrites store expression/leader handling. We no longer use the
value operand as the leader, instead, we store it separately. We also
now store the stored value as part of the expression, and compare it
when comparing stores for equality. This enables us to get rid of a
bunch of our previous hacks and machinations, as the existing
machinery takes care of everything *except* updating the stored value
on classes. The only time we have to update it is if the storecount
goes to 0, and when we do, we destroy it.

Since we no longer use the value operand as the leader, during elimination, we have to use the value operand. Doing this also fixes a bunch of store forwarding cases we were missing.

Any value operand we use is guaranteed to either be updated by previous eliminations, or minimized by future ones.

(IE the fact that we don't use the most dominating value operand when it's not a constant does not affect anything).

Sadly, this change also exposes that we didn't pay attention to the
output of the pr31594.ll test, as it also very clearly exposes the
same store leader bug we are fixing here.

(I added pr31682.ll anyway, but maybe we think that's too large to be useful)

On the plus side, propagate-ir-flags.ll now passes due to the
corrected store forwarding.

This change was 3 stage'd on darwin and linux, with the full test-suite.

Event Timeline

dberlin created this revision.Jan 20 2017, 1:33 AM

This also fixes 31698, and i added the testcase from there to loadforward.ll

Fix a few comments, update commit description to mention 31698

davide edited edge metadata.Jan 20 2017, 10:47 AM

This looks good, thanks for the very quick fix (a minor comment inline)

lib/Transforms/Scalar/NewGVN.cpp
1224–1235

Wheee for this going away.

test/Transforms/NewGVN/pr31686.ll
1–11

I really think this test can go away as the issue is covered by the other.

davide accepted this revision.Jan 20 2017, 10:49 AM

Also, some of the tests we inherited from GVN/ are currently testing too little, so I think I'm going to take a look at their output and improve the CHECK: lines for them in NewGVN if needed (sadly this is also true for other passes in Scalar/)

This revision is now accepted and ready to land.Jan 20 2017, 10:49 AM

And we should ask for 4.0 backport of this once this goes in.

davide closed this revision.Jan 22 2017, 9:00 PM