Page MenuHomePhabricator

[GVNHoist] Fix: PR35222 gvn-hoist incorrectly erases load in case of a loop
ClosedPublic

Authored by hiraditya on Dec 20 2017, 8:43 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

hiraditya created this revision.Dec 20 2017, 8:43 AM

There are a bunch of places in GVNHoist which call dominates (as opposed to properlyDominates), and some of them seem dubious. Are you going to audit them?

sebpop accepted this revision.Dec 21 2017, 7:55 AM

This change looks good.

For the other uses of dominates, you could add an assert(dominates == properlyDominates) that would allow you to automate the extraction of testcases with bugpoint when that is not the case.
We would then have to reason on a case by case whether dominates should be changed.

This revision is now accepted and ready to land.Dec 21 2017, 7:55 AM

There are a bunch of places in GVNHoist which call dominates (as opposed to properlyDominates), and some of them seem dubious. Are you going to audit them?

I'll audit them, thanks for the comment. If you can point which looks dubious to you, that'd help as well. I'll push this patch as it is and add followup patches if changes are required.

This revision was automatically updated to reflect the committed changes.
efriedma added inline comments.Jan 4 2018, 2:07 PM
llvm/trunk/lib/Transforms/Scalar/GVNHoist.cpp
823

This looks suspicious... specifically, it isn't clear what's supposed to happen when Inst->getParent() == HoistPt. Granted, I'm not sure that can happen.