This is an archive of the discontinued LLVM Phabricator instance.

NewGVN: Fix bug exposed by PR31761
ClosedPublic

Authored by dberlin on Jan 26 2017, 1:07 PM.

Details

Summary

This does not actually fix the testcase in PR31761 (discussion is
ongoing on the testcase), but does fix a bug it exposes, where stores
were not properly clobbering loads.

We accomplish this by unifying the memory equivalence infratructure
back into the normal congruence infrastructure, and then properly
destroying congruence classes when memory state leaders disappear.

Event Timeline

dberlin created this revision.Jan 26 2017, 1:07 PM
davide accepted this revision.Jan 26 2017, 1:35 PM

I reviewed this as part of https://reviews.llvm.org/D29149 (duplicated the comments here)

lib/Transforms/Scalar/NewGVN.cpp
707–711

*sigh*.

768

#if 1 can go away.

1168–1182

Thanks for this very explicative comment, I think it's very useful. Nit, dot at the end of the last sentence.

1247–1252

This can be removed.

This revision is now accepted and ready to land.Jan 26 2017, 1:35 PM
dberlin marked 2 inline comments as done.Jan 26 2017, 1:49 PM
dberlin added inline comments.
lib/Transforms/Scalar/NewGVN.cpp
707–711

Yeah, i think i'm going to try to push that in, because i think this invariant may discover more bugs.

Additionally, it *should* us turn back on a much simpler invariant for the memory accesses:
back in the day, we wanted to turn on the invariant that two stores with equivalent memory states must be in the same congruence class, and that is now true again (AFAIK)

Except for the above case :(

768

Yup

This revision was automatically updated to reflect the committed changes.