This is an archive of the discontinued LLVM Phabricator instance.

[LoopInterchange] Use getExitBlock()/getExitingBlock instead of manual impl.
ClosedPublic

Authored by fhahn on Apr 4 2018, 11:48 AM.

Details

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma added inline comments.Apr 16 2018, 12:14 PM
lib/Transforms/Scalar/LoopInterchange.cpp
764 ↗(On Diff #141015)

This is slightly more generous than getLoopLatchExitBlock; is it okay if the terminator of the exiting block is a switch or invoke? Or do we check that somewhere else?

865 ↗(On Diff #141015)

Is it actually possible for OuterExit to be null here?

fhahn updated this revision to Diff 143596.Apr 23 2018, 10:18 AM
fhahn retitled this revision from [LoopInterchange] Use LoopInfo::getExitBlock() instead of manual impl. to [LoopInterchange] Use getExitBlock()/getExitingBlock instead of manual impl..

Thank you very much for having a look Eli and sorry for the delay, I needed to find some time to investigate another failure in the test suite with this change. Together with D45970 and this patch, building the test-suite with LoopInterchange + LTO passes.

I've updated the code to make sure the exiting's block's terminator is a branch instruction.

fhahn added inline comments.Apr 23 2018, 10:24 AM
lib/Transforms/Scalar/LoopInterchange.cpp
764 ↗(On Diff #141015)

Thanks for pointing that out, it was not checked anywhere else. I added checks.

865 ↗(On Diff #141015)

I had a look and could not spot any other checks ensuring we have a unique exit block.

efriedma added inline comments.Apr 23 2018, 11:55 AM
lib/Transforms/Scalar/LoopInterchange.cpp
555 ↗(On Diff #143596)

What is this closing brace doing here?

Also, the indentation looks weird.

865 ↗(On Diff #141015)

processLoopList contains an identical check. And I think OuterLoop->getExitingBlock() == OuterLoop->getLoopLatch() && isa<BranchInst>(OuterLoop->getLoopLatch()->getTerminator()) implies the exit is unique.

fhahn updated this revision to Diff 143615.Apr 23 2018, 12:13 PM

Fix brace and remove null checks for getExitBlock()

fhahn added inline comments.Apr 23 2018, 12:16 PM
lib/Transforms/Scalar/LoopInterchange.cpp
555 ↗(On Diff #143596)

Argh, sorry about that. Should be fixed now

865 ↗(On Diff #141015)

There's one check to ensure the outermost loop in a nest has a unique exit block in processLoopList. Ah yes, the check Latch == single exiting block & isa<BranchInst> should guarantee that we have a unique exit block, thanks!

This revision is now accepted and ready to land.Apr 24 2018, 5:07 PM
This revision was automatically updated to reflect the committed changes.