This is an archive of the discontinued LLVM Phabricator instance.

[LoopUnroll] Fix a crash
ClosedPublic

Authored by skatkov on Dec 22 2020, 3:39 AM.

Details

Summary

Loop peeling as a last step triggers loop simplification and this
can change the loop structure. As a result all cashed values like
latch branch becomes invalid.

Patch re-structure the code to take into account the possible
changes caused by peeling.

Diff Detail

Event Timeline

skatkov created this revision.Dec 22 2020, 3:39 AM
skatkov requested review of this revision.Dec 22 2020, 3:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 22 2020, 3:39 AM
Meinersbur added inline comments.Dec 22 2020, 11:10 AM
llvm/lib/Transforms/Utils/LoopUnroll.cpp
365–370

Could you add a comment about that these have to be done after peelLoop?

370–371

[nit] re-add newline like the where it was moved from

llvm/test/Transforms/LoopUnroll/unroll-after-peel.ll
8

Consider removing !prof !0

48

Is this call required for the test?

skatkov updated this revision to Diff 313470.Dec 22 2020, 6:58 PM

Handled comments,

skatkov marked 3 inline comments as done.Dec 22 2020, 6:59 PM
skatkov added inline comments.
llvm/test/Transforms/LoopUnroll/unroll-after-peel.ll
48

Peeling guard requires that all non-latch exits must end-up by call to deoptimize.
So in other words peeling will not happen if I remove this call.

fhahn added inline comments.Dec 23 2020, 7:30 AM
llvm/lib/Transforms/Utils/LoopUnroll.cpp
404

Does this need updating? At this point, we could have peeled the loop and thus change it I think.

Also, by moving the early exit to after peeling, we now expose more cases to peeling. Can peeling handle the case of the early exit properly?

llvm/test/Transforms/LoopUnroll/unroll-after-peel.ll
6

the triple should not be needed?

40

Would it be possible to use a concrete condition here? (branch on undef is UB)

skatkov updated this revision to Diff 313656.Dec 23 2020, 10:26 PM

Test is updated.

skatkov marked 2 inline comments as done.Dec 23 2020, 11:18 PM
skatkov added inline comments.
llvm/lib/Transforms/Utils/LoopUnroll.cpp
404

I'm sorry I do not follow what you mean here. Could you please elaborate on what should be changed?

The only earlier return we do not do now for peeling is
!LatchBI || (LatchBI->isConditional() && !LatchIsExiting)

Loop peeling has its own guard canPeel() which is also triggered in computePeelCount.
Peeling guard requires that latch is a conditional exiting block.
So nothing new we are introducing.

fhahn added inline comments.Dec 28 2020, 5:10 AM
llvm/lib/Transforms/Utils/LoopUnroll.cpp
404

Loop peeling has its own guard canPeel() which is also triggered in computePeelCount.
Peeling guard requires that latch is a conditional exiting block.

If we have a similar check for peeling everything is fine, thanks for checking! Can you add an assertion here that we did not peel, so we can catch the case early when we need to update the returned status?

skatkov updated this revision to Diff 313926.Dec 28 2020, 7:52 PM

requested assert is added

Meinersbur accepted this revision.Jan 4 2021, 6:13 PM

LGTM, thank you.

llvm/lib/Transforms/Utils/LoopUnroll.cpp
365–366

[grammar] All these values should be taken only after peeling because they might have changed.

This revision is now accepted and ready to land.Jan 4 2021, 6:13 PM
fhahn accepted this revision.Jan 4 2021, 11:30 PM

Thanks for adding the assert! LGTM

This revision was automatically updated to reflect the committed changes.