Page MenuHomePhabricator

Loop peeling: check that latch is conditional branch
ClosedPublic

Authored by JosephTremoulet on Jan 19 2021, 12:49 PM.

Details

Summary

Loop peeling assumes that the loop's latch is a conditional branch. Add
a check to canPeel that explicitly checks for this, and a testcase that
otherwise fails an assertion when trying to peel a loop whose back-edge
is the non-unwind edge of an invoke.

Diff Detail

Event Timeline

JosephTremoulet requested review of this revision.Jan 19 2021, 12:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 19 2021, 12:49 PM

This is meant to address a regression from rG7f69860243e8, which moved a check for whether dynamic cast to BranchInst succeeds in UnrollLoop from before to after the call to peelLoop. Adding a check to canPeel seemed like the most logical option to me.

llvm/lib/Transforms/Utils/LoopPeel.cpp
664

Sorry, I jumped the gun in posting this -- this change makes the assertion here fail in several of the loop peeling lit tests. I'm taking a look...

  • Cast the terminator, not the block
llvm/lib/Transforms/Utils/LoopPeel.cpp
664

Sorry, I jumped the gun in posting this -- this change makes the assertion here fail in several of the loop peeling lit tests. I'm taking a look...

Fixed.

skatkov accepted this revision.Jan 19 2021, 6:52 PM
This revision is now accepted and ready to land.Jan 19 2021, 6:52 PM
fhahn added inline comments.Jan 20 2021, 2:44 AM
llvm/lib/Transforms/Utils/LoopPeel.cpp
123

this does only check for branch instructions, not conditional branches. Can you update the comment?

fhahn added a comment.Jan 20 2021, 3:56 AM

Looks like this issue has also been reported in https://bugs.llvm.org/show_bug.cgi?id=48812

  • Match comment to code
JosephTremoulet marked an inline comment as done.Jan 20 2021, 5:38 AM
JosephTremoulet added inline comments.
llvm/lib/Transforms/Utils/LoopPeel.cpp
123

this does only check for branch instructions, not conditional branches. Can you update the comment?

Updated.

fhahn accepted this revision.Jan 20 2021, 5:49 AM

LGTM, thanks! Could you also add the switch test case from PR48812?

JosephTremoulet marked an inline comment as done.
  • Add testcase from PR48812

Could you also add the switch test case from PR48812?

Done.

fhahn added a comment.Jan 20 2021, 6:43 AM

Could you also add the switch test case from PR48812?

Done.

Thanks. looks good