This is an archive of the discontinued LLVM Phabricator instance.

[GVNHoist] Fix: PR35222 gvn-hoist incorrectly erases load
ClosedPublic

Authored by hiraditya on Nov 7 2017, 10:29 PM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

hiraditya created this revision.Nov 7 2017, 10:29 PM
efriedma added inline comments.
lib/Transforms/Scalar/GVNHoist.cpp
802 ↗(On Diff #122030)

I haven't reviewed in detail, but I'm skeptical this check actually does the right thing in general. A "Loop*" is specifically a loop with a single entry point, so this check won't handle irreducible control flow correctly.

efriedma added inline comments.Nov 8 2017, 11:35 AM
lib/Transforms/Scalar/GVNHoist.cpp
802 ↗(On Diff #122030)

Looking a little more, it might not be relevant... I'm not following the algorithm for insertion points, and I don't really have time to dig deeper, so please ignore me. :)

hiraditya updated this revision to Diff 122266.Nov 9 2017, 10:28 AM

Added a testcase for irreducible control flow.

hiraditya marked an inline comment as done.Nov 9 2017, 10:30 AM
hiraditya added inline comments.
lib/Transforms/Scalar/GVNHoist.cpp
802 ↗(On Diff #122030)

Thanks!

dberlin added inline comments.Nov 9 2017, 10:34 AM
lib/Transforms/Scalar/GVNHoist.cpp
801 ↗(On Diff #122030)

I don't think this check is overall safe.

The definition of loop here is not going to be sufficient either.

I think if you want to avoid spurious CHI's, you need to do the equivalent of what you see in "A Practical Improvement to the Partial Redundancy Elimination in SSA Form" (https://sites.google.com/site/jongsoopark/home/ssapre.pdf)

which precisely defines which will be unnecessary and the safety conditions.

hiraditya added inline comments.Nov 10 2017, 4:21 PM
lib/Transforms/Scalar/GVNHoist.cpp
801 ↗(On Diff #122030)

Thanks for the reference, I'll study the paper and find out if they have some extra safety checks.

hiraditya updated this revision to Diff 125508.Dec 5 2017, 5:58 AM
hiraditya edited the summary of this revision. (Show Details)
This revision was not accepted when it landed; it landed in state Needs Review.Dec 13 2017, 11:40 AM
This revision was automatically updated to reflect the committed changes.