[llvm-commits] [llvm] r50382 - in /llvm/trunk: include/llvm/LinkAllPasses.h include/llvm/Transforms/Scalar.h lib/Transforms/Scalar/DeadLoopElimination.cpp

[Chris Lattner]

On Apr 28, 2008, at 5:38 PM, Owen Anderson wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=50382&view=rev
> Log:
> Add dead loop elimination, which removes dead loops for which we can  
> compute
> the trip count.

Nice!

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/lib/Transforms/Scalar/DeadLoopElimination.cpp (added)
> +++ llvm/trunk/lib/Transforms/Scalar/DeadLoopElimination.cpp Mon Apr  
> 28 19:38:34 2008
> @@ -0,0 +1,239 @@
> +//===- DeadLoopElimination.cpp - Dead Loop Elimination Pass  
> ---------------===//

All the other loop passes start with "Loop" is there some way to  
rename this to Loop*?  Maybe LoopDeletion?

> +#define DEBUG_TYPE "dead-loop"

Likewise all the options are -loop-xxx, it would be nice to be  
consistent.

>
> +
> +#include "llvm/Transforms/Scalar.h"
> +#include "llvm/Instruction.h"
> +#include "llvm/Analysis/LoopInfo.h"
> +#include "llvm/Analysis/LoopPass.h"

You don't need LoopInfo.h or Instruction.h

> +using namespace llvm;
> +
> +STATISTIC(NumDeleted, "Number of loops deleted");
> +
> +namespace {
> +  class VISIBILITY_HIDDEN DeadLoopElimination : public LoopPass {

Please add a nice big doxygen comment above this pass talking about  
what it does.  The key thing to talk about is that it deletes loops  
with no side effects that have a computable non-infinite iteration  
count.


> +bool DeadLoopElimination::SingleDominatingExit(Loop* L) {

Please add comments for every method, explaining what the method does  
and what invariants it expects from its input, like other passes in  
Transforms/Scalar.

> +bool DeadLoopElimination::IsLoopInvariantInst(Instruction *I, Loop*  
> L)  {

Can Loop::isLoopInvariant be used for this?

> +bool DeadLoopElimination::IsLoopDead(Loop* L) {
> +  SmallVector<BasicBlock*, 1> exitingBlocks;
> +  L->getExitingBlocks(exitingBlocks);
> +  BasicBlock* exitingBlock = exitingBlocks[0];
> +
> +  // Get the set of out-of-loop blocks that the exiting block  
> branches to.
> +  SmallVector<BasicBlock*, 8> exitBlocks;
> +  L->getUniqueExitBlocks(exitBlocks);
> +  if (exitBlocks.size() > 1)
> +    return false;
> +  BasicBlock* exitBlock = exitBlocks[0];

It seems that the property you really want is whether *any* exit from  
a loop that dominates the latch block has a computable trip count.   
Exits that don't dominate the latch block or that are not computable  
are fine, they just don't help prove finiteness.

... actually, I just realized that this isn't true.  You need much  
better comments explaining *why* you can't handle this case.


> +  // Make sure that all PHI entries coming from the loop are loop  
> invariant.
> +  BasicBlock::iterator BI = exitBlock->begin();
> +  while (PHINode* P = dyn_cast<PHINode>(BI)) {
> +    Value* incoming = P->getIncomingValueForBlock(exitingBlock);
> +    if (Instruction* I = dyn_cast<Instruction>(incoming))
> +      if (!IsLoopInvariantInst(I, L))

L->isLoopInvariant handles the non-instruction case fwiw.

> +bool DeadLoopElimination::runOnLoop(Loop* L, LPPassManager& LPM) {
> +  // Don't remove loops for which we can't solve the trip count.
> +  // They could be infinite, in which case we'd be changing program  
> behavior.
> +  if (L->getTripCount())
> +    return false;

You should move this check lower, it is more expensive to compute than  
whether there is a preheader or subloops.

> +  // We can only remove the loop if there is a preheader that we can
> +  // branch from after removing it.
> +  BasicBlock* preheader = L->getLoopPreheader();
> +  if (!preheader)
> +    return false;
> +
> +  // We can't remove loops that contain subloops.  If the subloops  
> were dead,
> +  // they would already have been removed in earlier executions of  
> this pass.
> +  if (L->begin() != L->end())
> +    return false;
> +
> +  // Loops with multiple exits or exits that don't dominate the latch
> +  // are too complicated to handle correctly.
> +  if (!SingleDominatingExit(L))
> +    return false;

Why?

>
> +
> +  // Finally, we have to check that the loop really is dead.
> +  if (!IsLoopDead(L))
> +    return false;
> +
> +  // Now that we know the removal is safe, change the branch from  
> the preheader
> +  // to go to the single exiting block.
> +  SmallVector<BasicBlock*, 1> exitingBlocks;
> +  L->getExitingBlocks(exitingBlocks);
> +  BasicBlock* exitingBlock = exitingBlocks[0];

You get the single exiting block 3 times for each loop.  Just get it  
once and pass it to methods that need it.

The comments you added to DeadLoopElimination::runOnLoop are very  
nice, thanks!


   // Connect the preheader directly to the exit block.
   TerminatorInst* TI = preheader->getTerminator();
   if (BranchInst* BI = dyn_cast<BranchInst>(TI)) {
     if (BI->isUnconditional())
       BI->setSuccessor(0, exitBlock);
     else if (L->contains(BI->getSuccessor(0)))
       BI->setSuccessor(0, exitBlock);
     else
       BI->setSuccessor(1, exitBlock);
   } else {
     // FIXME: Support switches
     return false;
   }

Your FIXME is not sufficient.  At this point you've already  
significantly hacked the function.  I'd just use TI- 
 >replaceUsesOfWith(oldblock, newblock), which should handle any  
terminator unless I'm missing something.  If I'm missing something,  
you have insufficient comments.



> +  BasicBlock::iterator BI = exitBlock->begin();
> +  while (PHINode* P = dyn_cast<PHINode>(BI)) {
> +    unsigned i = P->getBasicBlockIndex(exitingBlock);
> +    P->setIncomingBlock(i, preheader);

How about P->replaceUsesOfWith(oldblock, newblock)
>
> +    BI++;
> +  }



   // Update lots of internal structures...
   DominatorTree& DT = getAnalysis<DominatorTree>();
   for (Loop::block_iterator LI = L->block_begin(), LE = L->block_end();

Which internal structures?  This is not a very useful comment.

     SmallPtrSet<DomTreeNode*, 8> childNodes;

Please declare childNodes outside the loop so it doesn't get  
reconstructed on every iteration.  It should be named 'ChildNodes'.


> +  for (Loop::block_iterator LI = L->block_begin(), LE = L- 
> >block_end();
> +       LI != LE; ++LI) {
> +    for (BasicBlock::iterator BI = (*LI)->begin(), BE = (*LI)->end();
> +         BI != BE; ) {
> +      Instruction* I = BI++;
> +      I->eraseFromParent();
> +    }
> +
> +    (*LI)->eraseFromParent();
> +  }

You are incrementing LI after you erase *LI.  I don't think there is  
any need to explicitly erase the instructions in the block here, just  
nuking the block will delete the contents.

-Chris