Page MenuHomePhabricator

[LICM] Hoist guards with invariant conditions
ClosedPublic

Authored by mkazantsev on Aug 9 2018, 3:13 AM.

Details

Summary

This patch teaches LICM to hoist guards from the loop if they are guaranteed to execute and
if there are no side effects that could prevent that.

Diff Detail

Repository
rL LLVM

Event Timeline

Didn't look at the code, but this needs a test case that shows we don't hoist the second guard in

for (;;) {
  guard(loop_varying);
  gurad(loop_invariant);
}
reames requested changes to this revision.Aug 9 2018, 2:26 PM
reames added inline comments.
lib/Transforms/Scalar/LICM.cpp
461 ↗(On Diff #159886)

Bug: Can only start true if a) readonly loop or b) header block.

523 ↗(On Diff #159886)

hoist always returns true. Separately, change it to return void and update other caller.

This revision now requires changes to proceed.Aug 9 2018, 2:26 PM
mkazantsev planned changes to this revision.Aug 9 2018, 8:11 PM
mkazantsev added inline comments.
lib/Transforms/Scalar/LICM.cpp
461 ↗(On Diff #159886)

Indeed. Thanks for pointing out!

mkazantsev added inline comments.Aug 9 2018, 8:14 PM
lib/Transforms/Scalar/LICM.cpp
523 ↗(On Diff #159886)

Not sure if it's a good idea... What if it changes in the future? Besides, sink can actually return false, and it would be strange to have different signatures for these two.

mkazantsev updated this revision to Diff 160052.Aug 9 2018, 8:52 PM

Fixed bug in mem-write detection, added tests to exercise guard-after-guard scenarios.

Fixed test: underlying aliasing bug caused incorrect behavior on it.

reames accepted this revision.Aug 10 2018, 1:58 PM

LGTM w/mandatory separate cleanup changes as described.

lib/Transforms/Scalar/LICM.cpp
523 ↗(On Diff #160066)

Note for anyone reading: This can be trivially extended to llvm.invariant.start w/no uses. I thought about requiring that in this review, but decided a separate patch was better.

523 ↗(On Diff #159886)

This was not a suggestion. Please make the change. It can be done post commit, but I want this cleanup done. At the moment, you have control flow in this patch which is never executed and introduces non-trivial reasoning complexity for no value.

In general, the person who changes code in the future has to think about the implications of their changes. Future proofing never works and just adds complexity now.

This revision is now accepted and ready to land.Aug 10 2018, 1:58 PM
mkazantsev added inline comments.Aug 14 2018, 2:45 AM
lib/Transforms/Scalar/LICM.cpp
523 ↗(On Diff #159886)
This revision was automatically updated to reflect the committed changes.