This is an archive of the discontinued LLVM Phabricator instance.

[Loop Deletion] Delete loops that are never executed
ClosedPublic

Authored by anna on Apr 25 2017, 10:03 AM.

Details

Summary

Currently, loop deletion only supports deleting loops that have backedges that
are not-taken into the loop header (i.e. the values outside the loop are invariant
wrt each loop iteration).
This patch adds logic to delete loops where the loop is proven to be
semantically unreachable.
The basic purpose here is to support and test the loop deletion
logic for the usage in loop-simplifyCFG, where changing constant conditional
branches to unconditional can make loops dead. This is the first set of changes to
merge loop-deletion and loop-simplifyCFG [1].

The next steps are:

  1. moving the loop deletion implementation to LoopUtils
  2. Add logic in loop-simplifyCFG which will support changing conditional

constant branches to unconditional branches. If loops become unreachable in this
process, they can be removed using deleteDeadLoop function.

[1] https://reviews.llvm.org/D32353#734363

Diff Detail

Repository
rL LLVM

Event Timeline

anna created this revision.Apr 25 2017, 10:03 AM
anna edited the summary of this revision. (Show Details)Apr 25 2017, 10:44 AM
sanjoy requested changes to this revision.Apr 25 2017, 11:12 AM

I made a few first pass comments. I think the term "semantically unreachable" sounds more complicated than it actually is. :) How about "never executed", as in isLoopNeverExecuted?

lib/Transforms/Scalar/LoopDeletion.cpp
48 ↗(On Diff #96586)

Indent seems off - clang-format?

119 ↗(On Diff #96586)

I think this distinction between predecessor and preheader is making the logic a bit more complex than it needs to be. I'd instead do:

auto *Pred = L->getLoopPredecessor();
auto *Succ = L->getHeader();
if (Pred && Pred->getTerminator() is not conditional branch) {
  Succ = Pred;
  Pred = Pred->getSinglePredecessor();
}
if (!Pred || Pred->getTerminator() is not conditional branch)
  return false;
134 ↗(On Diff #96586)

Why is Loop-SimplifyCFG relevant here? Won't it only simplify control flow within a loop?

137 ↗(On Diff #96586)

The T seems a bit superfluous here -- why not just auto *BI = dyn_cast<BranchInst>(HeaderPred->getTerminator())?

145 ↗(On Diff #96586)

What if both the branches branch to the header?

146 ↗(On Diff #96586)

This can be just return NotTaken == FirstLoopBlock;

189 ↗(On Diff #96586)

I'm missing where you're checking for this second case?

279 ↗(On Diff #96586)

How do dead phis make a block dead?

If you're talking about keeping the edge from SourceBB to ExitBlock even though it will never be taken, then I see what you mean, but that comment should be on SourceBB->getTerminator()->replaceUsesOfWith(L->getHeader(), ExitBlock);. Btw, what is the problem with deleting that edge? Is it with updating LoopInfo or something else?

This revision now requires changes to proceed.Apr 25 2017, 11:12 AM
anna marked 2 inline comments as done.Apr 25 2017, 2:38 PM
anna added inline comments.
lib/Transforms/Scalar/LoopDeletion.cpp
134 ↗(On Diff #96586)

Yes, that's right. The comment maybe confusing. My assumption is that these kind of constant conditional branches are generated by unswitch [1], i.e. branch is within a loop. At a later point, deleting unreachable loop will be handled by Loop-SimplifyCFG: It will *start* at a constant branch condition, iterating forward through the blocks that will never be executed, and in the process if we reach a loop header which will never be executed, that loop will be deleted.

So, loop-deletion will just be a subcase of loop-simplifyCFG. At some point, we should just be able to call loop-simplifyCFG in the pass pipeline instead of loop-deletion.

[1] If we have constant conditional branches outside of loops generated by some other non-loop passes, we could just use SimplifyCFG to eliminate the code? This brings me to the main reason for the patch: implementing the actual loop deletion, update dom tree etc for unreachable loops, and use that function for Loop-SimplifyCFG.

145 ↗(On Diff #96586)

ah, thanks :)

189 ↗(On Diff #96586)

That's the original and already existing case for loop deletion pass: see isLoopDead. I'll update this comment so that it does not look like this was an addition in the patch.

279 ↗(On Diff #96586)

This comment is incorrect. There's no problem in deleting the edge. The only reason is to keep the code here similar in both cases: keep the edge from the source block to the exit block. If I special case the "loop never executes" scenario to delete the edge, we can remove the value from the phi as well.
For now, I'll keep the code the same, and leave the edge as-is.

anna marked 2 inline comments as done.Apr 25 2017, 2:45 PM
anna added inline comments.
lib/Transforms/Scalar/LoopDeletion.cpp
134 ↗(On Diff #96586)

Actually, with loop-unswitch, the branch can be outside the loop :) So, the merging of loop-simplifyCFG and this version of loop-deletion should fix cases handled by both.
I'll remove the comment about loop-simplifyCFG. I guess we could have something like this in the pipeline:

unswitch
loop-deletion + loop-simplifycfg which handles constant conditional branches (both outside and within loops) that make some loop dead.
anna updated this revision to Diff 96927.Apr 27 2017, 9:21 AM
anna edited edge metadata.
anna marked 2 inline comments as done.

Addressed review comments. Added more tests with subloops and preserving loop structure.
Updated source code comments.

anna added inline comments.Apr 27 2017, 9:27 AM
lib/Transforms/Scalar/LoopDeletion.cpp
279 ↗(On Diff #96586)

just FYI, it looks like there is a problem in deleting the edge. If this loop is within a parent loop, we may break the parent loop's structure.
Consider:

L1:
  br i1 true, label %exit, label %L2 
L2:
  br i1 %cond, label %L2, label %L1

Here we can delete L2. If we remove the edge from the sourceBlock L1 to the exit block (which will be L1.exit or L1), the loop structure of L1 is no longer preserved. I have added a test case (see test8) in the updated diff, and updated the comment as well.

In test8, both the loops are deleted through iterative calls to loop deletion starting from inner loop to outer loop.

anna retitled this revision from [Loop Deletion] Delete loops that are semantically unreachable to [Loop Deletion] Delete loops that are never executed.Apr 27 2017, 9:29 AM
sanjoy requested changes to this revision.Apr 27 2017, 1:15 PM

Comments inline.

lib/Transforms/Scalar/LoopDeletion.cpp
34 ↗(On Diff #96927)

80 chars line?

105 ↗(On Diff #96927)

Is "not unreachable" == "i.e. it's header would always have atleast one predecessor" or == "its header must have a path from the entry block"?

107 ↗(On Diff #96927)

I'd rephrase this a bit: However, if the loop header would never be executed at runtime, the loop is considered semantically unreachable.

But generally, I'd be in favor of not adding so much detail here, but instead just saying something brief like: "This function returns true if there is no viable path from the entry block to the header of \p L. Right now it only does a local search to save compile time(?)"

134 ↗(On Diff #96927)

Perhaps this can be made more concise using PatternMatch?

BasicBlock *Taken, *NotTaken;
ConstantInst *Cond;
if (!match(Pred->getTerminator(), m_Br(m_ConstantInt(Cond), Taken, NotTaken)))
  return false;
if (!Cond->getZExtValue())
  std::swap(Taken, NotTaken);

edit: actually, I think this can be combined with the previous check too to make the entire function body be:

auto *Pred = L->getLoopPredecessor();
auto *FirstLoopBlock = L->getHeader();

for (int i = 0; i < 2 && Pred; i++) {
  BasicBlock *Taken, *NotTaken;
  ConstantInst *Cond;
  if (match(Pred->getTerminator(), m_Br(m_ConstantInt(Cond), Taken, NotTaken))) {
    if (!Cond->getZExtValue())
      std::swap(Taken, NotTaken);
    return NotTaken == FirstLoopBlock && Taken != FirstLoopBlock;
    // Alternatively:
    // if (NotTaken == FirstLoopBlock && Taken != FirstLoopBlock)
    //   return true;
  }
  FirstLoopBlock = Pred;
  Pred = Pred->getSinglePredecessor();
}
188 ↗(On Diff #96927)

I didn't understand the "has no backedge" part -- if it does not have a backedge how is it a loop?

224 ↗(On Diff #96927)

Why not insert a preheader if there wasn't one (by splitting the edge from the loop predecessor to the header), and keep the rest of the CFG modifying logic the same? IIUC, the only other place that needs to change is the P->setIncomingValue(j, UndefValue::get(P->getType())), and even that can be done as a pre-pass before deleteDeadLoop keeping the core logic here the same in both the situations.

235 ↗(On Diff #96927)

I would just pass in the block that we decided was the last executed block instead of re-deriving it here. Otherwise we risk the logic here and isLoopNeverExecuted diverging.

[edit: the comment above about splitting the preheader supersedes this comment]

This revision now requires changes to proceed.Apr 27 2017, 1:15 PM
anna added inline comments.Apr 27 2017, 3:01 PM
lib/Transforms/Scalar/LoopDeletion.cpp
188 ↗(On Diff #96927)

Actually, what I meant was it's "semantically not having a backedge": in isLoopDead, we check to see that only invariant values from the loop are used in the exit block. Then we hoist these invariant values *and their operands* to the preheader. I think this is equivalent to "semantically not having a backedge". I'm just going to remove these comments instead of going into all this detail :)

But just for my own clarification, do you think equivalent, or my statement is a stronger version of isLoopDead?

235 ↗(On Diff #96927)

Actually, that's what I did initially (during local modification). However, it required passing that last executed block between 3 functions. I personally didnt like that design either.

I like your first comment on splitting the edge from the loop predecessor to the header. I'll try to get that working and keep the core logic as similar as possible.

sanjoy added inline comments.Apr 27 2017, 3:04 PM
lib/Transforms/Scalar/LoopDeletion.cpp
188 ↗(On Diff #96927)

But just for my own clarification, do you think equivalent, or my statement is a stronger version of isLoopDead?

I believe the code is checking for loops like this:

int x = ..., y = ...;
int z;
for (int i = 0; i < 5; i++)
  z = x + y;
use(z);

I'd say the loop above does have a backedge but you're right in pointing out that the backedge is not "useful".

anna updated this revision to Diff 97295.May 1 2017, 8:36 AM
anna edited edge metadata.
anna marked 3 inline comments as done.

Addressed review comments.
Main changes: Preheader is now a requirement for never executed loops.
We can also handle multiple immediate predecessors in the never executed loop.

anna marked an inline comment as done.May 1 2017, 8:43 AM

In this change, I've added the preheader requirement to never executed loops as well. The main reason for adding this requirement
is that in all testcases I've added, opt -loop-deletion always generates a preheader for the loop before running this pass.
Not sure how to test for non-preheader cases.
So, the core logic in deleteDeadLoop is now very similar to initial code before the patch.

lib/Transforms/Scalar/LoopDeletion.cpp
105 ↗(On Diff #96927)

the latter. i've updated the comment.

134 ↗(On Diff #96927)

I've changed this function to consider all immediate predecessors of the preheader. Preheader is now a requirement for this function. Pls see the updated function.

235 ↗(On Diff #96927)

This logic is now removed because of the preheader requirement.

sanjoy requested changes to this revision.May 1 2017, 4:00 PM

This looks pretty close to done.

One high level issue I have is that you've interchangeably used "semantically unreachable" and "never executed". I'd standardize to one or the other to reduce confusion. I'd lean towards "never executed" or "known never executed", but if you find "semantically unreachable" better, using that would also be better than using both.

lib/Transforms/Scalar/LoopDeletion.cpp
35 ↗(On Diff #97295)

I thought we decided that backedges do exist in those cases?

37 ↗(On Diff #97295)

Isn't the second kind of loop still a loop? It just is a loop that is provably not reachable.

39 ↗(On Diff #97295)

Why not s/be more aggressive in removing the/always remove/ ?

40 ↗(On Diff #97295)

s/program behaviour (observable or unobservable)/observable program behavior/

I don't think we care about changing unobservable program behavior. :)

I'd even drop the observable, btw, since "program behavior" means "observable program behavior".

135 ↗(On Diff #97295)

Is the former ("the preheader has no predecessors") possible since we have an llvm::Loop? If not, I'd add an assert.

156 ↗(On Diff #97295)

Any reason why you reordered the preheader and the empty loop checks? If not, I'd lean towards to keeping them the way they are to make the diff smaller.

254 ↗(On Diff #97295)

Not directly relevant to this patch, but is this bit correct with multiple edges from an exiting block to an exit block?

336 ↗(On Diff #97295)

Debugging code?

test/Transforms/LoopDeletion/unreachable-loops.ll
1 ↗(On Diff #97295)

Why not just add -verify-dom-info to the first invocation (instead of having a separate one)?

This revision now requires changes to proceed.May 1 2017, 4:00 PM
anna marked 7 inline comments as done.May 2 2017, 7:52 AM
anna added inline comments.
lib/Transforms/Scalar/LoopDeletion.cpp
35 ↗(On Diff #97295)

oops, forgot to change the comment.

37 ↗(On Diff #97295)

It is, but I think it's no longer a loop according to how llvm identifies loops. See LoopInfoImpl.h analyze: We always check that the backedges are reachable from entry (which auto implies the header that dominates these backedges are also reachable from entry). I've made the description of this function more concise though.

135 ↗(On Diff #97295)

I think the only case is when the preheader is the function entry block. That's already been handled above. So, per llvm notion, the preheader should have predecessors at this point. Added the assert.

254 ↗(On Diff #97295)

I think this is correct because the ExitingBlocks are generated by getExitingBlocks, so it handles multiple edges from an exiting block to an exit block. If it was getUniqueExitingBlocks, this would be incorrect.

336 ↗(On Diff #97295)

thanks :)

anna updated this revision to Diff 97455.May 2 2017, 8:06 AM
anna edited edge metadata.
anna marked 3 inline comments as done.

Main change: Updated comments to uniformly use terminology as loop is never executed
instead of semantically unreachable.
Addressed other review comments.

sanjoy accepted this revision.May 2 2017, 9:50 AM

lgtm

This revision is now accepted and ready to land.May 2 2017, 9:50 AM
This revision was automatically updated to reflect the committed changes.