[llvm-commits] [llvm] r50382 - in /llvm/trunk: include/llvm/LinkAllPasses.h include/llvm/Transforms/Scalar.h lib/Transforms/Scalar/DeadLoopElimination.cpp
Chris Lattner
clattner at apple.com
Tue Apr 29 11:36:06 PDT 2008
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
More information about the llvm-commits
mailing list