This is an archive of the discontinued LLVM Phabricator instance.

[LoopUnroll] Avoid branch on poison for runtime unroll with multiple exits
ClosedPublic

Authored by nikic on May 17 2022, 2:32 AM.

Details

Summary

When performing runtime unrolling with multiple exits, one of the earlier (non-latch) exits may exit the loop on the first iteration, such that we never branch on the latch exit condition. As such, we need to freeze the condition of the new branch that is introduced before the loop, as it now executed unconditionally.

Diff Detail

Event Timeline

nikic created this revision.May 17 2022, 2:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 2:32 AM
nikic requested review of this revision.May 17 2022, 2:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 17 2022, 2:32 AM
nikic retitled this revision from [LoopUnroll] Avoid branch on poison with multiple exits to [LoopUnroll] Avoid branch on poison for runtime unroll with multiple exits.May 17 2022, 2:33 AM
reames added inline comments.May 17 2022, 8:15 AM
llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp
755

Its not clear to me that you need this. The condition being branched on should always be derived from the SCEV computed trip count. After D124910, shouldn't we be assured that is non-poisonous? Or at least, only poisonous if the first input is poison?

Your code here is completely reasonable, except that I don't quite understand the case in which is matters. :)

nikic added inline comments.May 17 2022, 8:21 AM
llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp
755

Runtime loop unrolling doesn't use the backedge taken count (which is what D124910 is about), it works only on the latch exit count. So if you have two exits at n and m (with m being the latch exit), runtime unroll will unroll using m, not umin(n, m). That's why we need to handle this explicitly, it wouldn't be going through the umin_seq code.

reames accepted this revision.May 17 2022, 8:40 AM

LGTM

llvm/lib/Transforms/Utils/LoopUnrollRuntime.cpp
755

You are correct. Odd, I thought we'd changed that previously, but it looks like we haven't.

This revision is now accepted and ready to land.May 17 2022, 8:40 AM
This revision was landed with ongoing or failed builds.May 18 2022, 12:51 AM
This revision was automatically updated to reflect the committed changes.