This is an archive of the discontinued LLVM Phabricator instance.

[LICM] Use per-block hoist points when rehoisting instructions
ClosedPublic

Authored by john.brawn on Dec 4 2018, 6:14 AM.

Details

Summary

In some cases the order that we hoist instructions in means that when rehoisting (which uses the same order as hoisting) we can rehoist to a block A, then a block B, then block A again. This currently causes an assertion failure as it expects that when changing the hoist point it only ever moves to a block that dominates the hoist point being moved from.

Fix this by having per-block hoist points, which means we can do away with the assertion.

Diff Detail

Repository
rL LLVM

Event Timeline

john.brawn created this revision.Dec 4 2018, 6:14 AM
mkazantsev added inline comments.Dec 4 2018, 7:50 PM
lib/Transforms/Scalar/LICM.cpp
810

If we replace HoistPoint->getParent() != Dominator with DT->dominates(Dominator, HoistPoint->getParent()), will it solve your problem?

The entire algorithm looks non-deterministic and depends on traversal order of uses. I suggest making it deterministic (I think my comment inline should help).

john.brawn updated this revision to Diff 176976.Dec 6 2018, 7:56 AM
john.brawn marked an inline comment as done.

Adjust according to review comments, and also add some more tests.

This revision is now accepted and ready to land.Dec 24 2018, 1:01 PM
This revision was automatically updated to reflect the committed changes.
john.brawn marked an inline comment as not done.