Page MenuHomePhabricator

[SCEV] Adding a check on outgoing branches of a terminator instr for SCEVBackedgeConditionFolder, NFC.
ClosedPublic

Authored by jbhateja on Nov 25 2017, 5:56 PM.

Details

Summary

For a given loop, getLoopLatch returns a non-null value
when a loop has only one latch block. In the modified
context adding an assertion to check that both the outgoing branches of
a terminator instruction (of latch) does not target same header.
+
few minor code reorganization.

Diff Detail

Repository
rL LLVM

Event Timeline

jbhateja created this revision.Nov 25 2017, 5:56 PM
jbhateja accepted this revision.Nov 25 2017, 5:57 PM
This revision is now accepted and ready to land.Nov 25 2017, 5:57 PM
This revision was automatically updated to reflect the committed changes.
sanjoy added inline comments.Nov 25 2017, 7:53 PM
llvm/trunk/lib/Analysis/ScalarEvolution.cpp
4095–4097

Can you please add an assert here that BI->getSuccessor(0) != BI->getSuccessor(1)? That way we'll know to update this place if/when getLoopLatch() becomes smarter.

jbhateja added inline comments.Nov 25 2017, 8:33 PM
llvm/trunk/lib/Analysis/ScalarEvolution.cpp
4095–4097

I guess we will have to look at all the places from where a call to getLoopLatch is made, adding such an assert at one place does not looks reasonable.

sanjoy added inline comments.Nov 26 2017, 12:03 AM
llvm/trunk/lib/Analysis/ScalarEvolution.cpp
4095–4097

adding such an assert at one place does not looks reasonable

I'm not sure I agree. Here we rely on BI->getSuccessor(0) != BI->getSuccessor(1) implied from getLoopLatch() for correctness, and asserting that precondition seems like an incremental improvement, irrespective of what we do for the other places.

Secondly, I don't think asserting BI->getSuccessor(0) != BI->getSuccessor(1) everywhere we call getLoopLatch() is a good idea anyway. We should assert BI->getSuccessor(0) != BI->getSuccessor(1) where it matters to the code that called getLoopLatch(), not to all places that call getLoopLatch().

jbhateja reopened this revision.Nov 26 2017, 5:03 AM
This revision is now accepted and ready to land.Nov 26 2017, 5:03 AM
jbhateja updated this revision to Diff 124304.Nov 26 2017, 6:29 AM

Few more minor changes , mostly NFCs.

jbhateja retitled this revision from [SCEV] NFC : Removing unnecessary check on outgoing branches of a branch instr. to [SCEV] Adding a check on outgoing branches of a terminator instr for SCEVBackedgeConditionFolder, NFC..Nov 26 2017, 6:40 AM
jbhateja edited the summary of this revision. (Show Details)
This revision was automatically updated to reflect the committed changes.