This is an archive of the discontinued LLVM Phabricator instance.

[LICM] Report failing to hoist a load with an invariant address
ClosedPublic

Authored by anemet on Dec 19 2016, 1:36 PM.

Details

Summary

These are interesting because lack of precision in alias information
could be standing in the way of this optimization.

An example is the case in the test suite that I showed in the DevMeeting
talk:

http://lab.llvm.org:8080/artifacts/opt-view_test-suite/build/MultiSource/Benchmarks/FreeBench/distray/CMakeFiles/distray.dir/html/_org_test-suite_MultiSource_Benchmarks_FreeBench_distray_distray.c.html#L236

canSinkOrHoistInst is also used from LoopSink, which does not use
opt-remarks so we need to take ORE as an optional argument.

Event Timeline

anemet updated this revision to Diff 81993.Dec 19 2016, 1:36 PM
anemet retitled this revision from to [LICM] Report failing to hoist a load with an invariant address.
anemet updated this object.
anemet added a reviewer: hfinkel.
anemet added a subscriber: llvm-commits.
hfinkel added inline comments.Jan 8 2017, 12:49 PM
lib/Transforms/Scalar/LICM.cpp
478

Can we hoist a load without a loop-invariant address? It seems like we shouldn't be making aliasing queries for loads we can't possibly hoist, and so I don't understand why we specifically need the CurLoop->isLoopInvariant check here.

anemet added inline comments.Jan 10 2017, 11:08 AM
lib/Transforms/Scalar/LICM.cpp
478

No, you're right. The call to this is guarded with hasLoopInvariantOperands.

anemet added inline comments.Jan 10 2017, 2:33 PM
lib/Transforms/Scalar/LICM.cpp
478

Actually, I take this back. We need the check. See the new comment in the new version of the patch and also the new testcase for an example.

anemet updated this revision to Diff 83875.Jan 10 2017, 2:35 PM

Add comment and testcase to explain why we need to check for the
loop-invariance of the pointer.

hfinkel added inline comments.Jan 10 2017, 4:13 PM
lib/Transforms/Scalar/LICM.cpp
478

Okay, but I wonder if this points to a larger problem. Maybe we just need a FIXME on this comment for now, but I'm thinking that this invariance check is cheap and the other checks (i.e. aliasing queries, dereferenceablility checks, etc.) are, at least in some cases, expensive. Thus, this guard really belongs elsewhere. We can sink something with a non-loop-invariant store either, can we?

anemet added inline comments.Jan 10 2017, 4:49 PM
lib/Transforms/Scalar/LICM.cpp
478

Yes but we *can* sink a load with a loop-variant address whose result is only used outside the loop (see testcase).

Loop-invariance is not tested in this case (because it shouldn't) however it is tested on other paths to this code (as it should) so I don't think there is a problem of wasting work.

hfinkel accepted this revision.Jan 10 2017, 4:52 PM
hfinkel edited edge metadata.
hfinkel added inline comments.
lib/Transforms/Scalar/LICM.cpp
478

Ah, okay. That makes sense, thanks! LGTM.

This revision is now accepted and ready to land.Jan 10 2017, 4:52 PM
This revision was automatically updated to reflect the committed changes.