This is an archive of the discontinued LLVM Phabricator instance.

LICM: Don't sink stores out of loops that may throw.
ClosedPublic

Authored by eli.friedman on Jun 2 2016, 7:05 PM.

Details

Summary

This hasn't been caught before because it requires noalias or similarly
strong alias analysis to actually reproduce.

Fixes http://llvm.org/PR27952 .

Diff Detail

Repository
rL LLVM

Event Timeline

eli.friedman retitled this revision from to LICM: Don't sink stores out of loops that may throw..
eli.friedman updated this object.
eli.friedman added reviewers: hfinkel, sanjoy.
eli.friedman added a subscriber: llvm-commits.
hfinkel added inline comments.Jun 2 2016, 9:02 PM
lib/Transforms/Scalar/LICM.cpp
936 ↗(On Diff #59485)

The assumption here is that stores to allocas much be unobservable after the function unwinds, and I agree with that. However, what if the exception is caught in this function. In that case, such a store might still be observable.

Also, it would be friendlier to call GetUnderlyingObjects and check whether any are not allocas (if we can determine that there are no relevant landingpads?).

eli.friedman added inline comments.Jun 2 2016, 11:33 PM
lib/Transforms/Scalar/LICM.cpp
936 ↗(On Diff #59485)

The tricky case here is specifically call instructions, which unwind without an explicit unwind edge . If there's an invoke instruction in a loop, the unwind edge is visible to the optimizer; we can treat it the same way as any other loop exit.

Changing this to use GetUnderlyingObjects seems fine.

eli.friedman added inline comments.Jun 3 2016, 12:01 AM
lib/Transforms/Scalar/LICM.cpp
936 ↗(On Diff #59485)

Hmm... actually, just tried GetUnderlyingObjects; can't get it to do anything useful because alias analysis can't figure out that x ? &a2 : &a2 doesn't alias an arbitrary function call.

hfinkel accepted this revision.Jun 3 2016, 4:17 AM
hfinkel edited edge metadata.
hfinkel added inline comments.
lib/Transforms/Scalar/LICM.cpp
936 ↗(On Diff #59485)

Okay, we should fix that too :( - This LGTM.

This revision is now accepted and ready to land.Jun 3 2016, 4:17 AM
This revision was automatically updated to reflect the committed changes.