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

[Chris Lattner]

> URL: http://llvm.org/viewvc/llvm-project?rev=40883&view=rev
> Log:
> Begin loop index split pass.

Nice!

>
> +// 
> ===------------------------------------------------------------------- 
> ---===//
> +//
> +// LoopIndexSplit - This pass splits loop

Please finish your sentence :)

> +//
> +LoopPass *createLoopIndexSplitPass();
> +
>
> ====================================================================== 
> ========
> --- llvm/trunk/lib/Transforms/Scalar/LoopIndexSplit.cpp (added)
> +++ llvm/trunk/lib/Transforms/Scalar/LoopIndexSplit.cpp Mon Aug  6  
> 19:25:56 2007
> @@ -0,0 +1,384 @@
..
> +
> +#define DEBUG_TYPE "loop-index-split"
> +
> +#include "llvm/Function.h"
> +#include "llvm/Transforms/Scalar.h"

This should include Transforms/Scalar first, because it is the  
"interface" to the .cpp file.

> +/// Find condition inside a loop that is suitable candidate for  
> index split.
> +void LoopIndexSplit::findSplitCondition() {
> +
> + BasicBlock *Header = L->getHeader();
> +
> +  for (BasicBlock::iterator I = Header->begin(); isa<PHINode>(I); + 
> +I) {
> +    PHINode *PN = cast<PHINode>(I);
> +
> +    if (!PN->getType()->isInteger())
> +      continue;
> +
> +    SCEVHandle SCEV = SE->getSCEV(PN);
> +    if (!isa<SCEVAddRecExpr>(SCEV))
> +      continue;
> +
> +    // If this phi node is used in a compare instruction then it is a
> +    // split condition candidate.
> +    for (Value::use_iterator UI = PN->use_begin(), E = PN->use_end();
> +         UI != E; ++UI) {
> +      if (ICmpInst *CI = dyn_cast<ICmpInst>(*UI)) {
> +        SplitCondition = CI;
> +        break;
> +      }
> +    }

One problem with this is that it won't find conditions that are  
derived expressions of the induction variable.  Consider "if (iv+1 ==  
17)".  Because the use of the IV is actually the add, this won't pick  
it up.

I'd suggest scanning the terminators in the loop body, looking for  
comparisons where one side is a loop invariant, and one side is an  
addrecexpr corresponding to this loop.

Another thing to consider: in the "non-one-iteration loop" case, you  
can actually have multiple indexes to split.  The ultimate  
implementation will probably want to collect all of them for a loop  
and use a cost model to determine which is the cheapest to split  
(based on how much code would have to be duplicated).

> +  // Replace split condition in header.
> +  // Transform
> +  //      SplitCondition : icmp eq i32 IndVar, SplitValue
> +  // into
> +  //      c1 = icmp uge i32 SplitValue, StartValue
> +  //      c2 = icmp ult i32 vSplitValue, ExitValue
> +  //      and i32 c1, c2
> +  bool SignedPredicate = SplitCondition->isSignedPredicate();

I don't think this is right.  Doesn't the signedness of the compare  
depend on the loop exit condition, not the split condition?

while (i < n)
   if (i == 17)

You want the signedness of the i < n comparison.

Similarly, somewhere in the safety checks, you should verify that the  
exit condition really is an integer compare.

> +  Instruction *C1 = new ICmpInst(SignedPredicate ?
> +                                 ICmpInst::ICMP_SGE :  
> ICmpInst::ICMP_UGE,
> +                                 SplitValue, StartValue,  
> "lisplit", Terminator);
> +  Instruction *C2 = new ICmpInst(SignedPredicate ?
> +                                 ICmpInst::ICMP_SLT :  
> ICmpInst::ICMP_ULT,
> +                                 SplitValue, ExitValue, "lisplit",  
> Terminator);
> +  Instruction *NSplitCond = BinaryOperator::create(Instruction::And,
> +                                                   C1, C2,  
> "lisplit", Terminator);

Plz use BinaryOperator::createAnd(C1, C2, "lisplit", Terminator);

> +  SplitCondition->replaceAllUsesWith(NSplitCond);
> +  SplitCondition->removeFromParent();
> +  delete SplitCondition;

Instead of removeFromParent + delete, just use eraseFromParent().

> +  BranchInst *BR = dyn_cast<BranchInst>(Latch->getTerminator());
> +  BR->setUnconditionalDest(LatchSucc);

If you know the terminator is a branch, use cast<BranchInst>,  
otherwise you need to check to see if BR is null.

> +  // Now, clear latch block. Remove instructions that are responsible
> +  // to increment induction variable.
> +  Instruction *LTerminator = Latch->getTerminator();
> +  for (BasicBlock::iterator LB = Latch->begin(), LE = Latch->end();
> +       LB != LE; ) {
> +    Instruction *I = LB;
> +    ++LB;
> +    if (isa<PHINode>(I) || I == LTerminator)
> +      continue;
> +
> +    I->replaceAllUsesWith(UndefValue::get(I->getType()));

This won't work if I has void type.  I don't know if that is possible  
though, do to your safety predicate.

> +    I->removeFromParent();
> +    delete I;

These two lines can be I->eraseFromParent();

> +// If loop header includes loop variant instruction operands then
> +// this loop can not be eliminated. This is used by  
> processOneIterationLoop().
> +bool LoopIndexSplit::safeHeader(BasicBlock *Header) {
> +
> +  Instruction *Terminator = Header->getTerminator();
> +  for(BasicBlock::iterator BI = Header->begin(), BE = Header->end();
> +      BI != BE; ++BI) {
> +    Instruction *I = BI;
> +
> +    // PHI Nodes are OK.
> +    if (isa<PHINode>(I))
> +      continue;

What if the phi node is live out of the loop?

   for (i = 0, j = 0; i < n; ++i, j = i)
     if (i == 123) ...

   use(j)

?

> +
> +    // SplitCondition itself is OK.
> +    if (ICmpInst *CI = dyn_cast<ICmpInst>(I)) {
> +      if (CI == SplitCondition)
> +        continue;
> +    }

No need for the dyncast, just use:

> +    if (CI == SplitCondition)
> +      continue;


> +// If Exit block includes loop variant instructions then this
> +// loop may not be eliminated. This is used by  
> processOneIterationLoop().
> +bool LoopIndexSplit::safeExitBlock(BasicBlock *ExitBlock) {
> +
> +  Instruction *ExitCondition = NULL;
> +  Instruction *IndVarIncrement = NULL;
> +
> +  for (BasicBlock::iterator BI = ExitBlock->begin(), BE =  
> ExitBlock->end();
> +       BI != BE; ++BI) {
> +    Instruction *I = BI;
> +
> +    // PHI Nodes are OK.
> +    if (isa<PHINode>(I))
> +      continue;

similar to the above.

> +    // Check if I is induction variable increment instruction.
> +    if (BinaryOperator *BOp = dyn_cast<BinaryOperator>(I)) {
> +      if (BOp->getOpcode() != Instruction::Add)
> +        return false;

No need for the dyncast:

> +    if (I->getOpcode() != Instruction::Add)
> +      return false;

should work.


> +      Value *Op0 = BOp->getOperand(0);
> +      Value *Op1 = BOp->getOperand(1);
> +      PHINode *PN = NULL;
> +      ConstantInt *CI = NULL;
> +
> +      if ((PN = dyn_cast<PHINode>(Op0))) {
> +        if ((CI = dyn_cast<ConstantInt>(Op1)))
> +          IndVarIncrement = I;
> +      } else
> +        if ((PN = dyn_cast<PHINode>(Op1))) {
> +          if ((CI = dyn_cast<ConstantInt>(Op0)))
> +            IndVarIncrement = I;
> +      }
> +
> +      if (IndVarIncrement && PN == IndVar && CI->isOne())
> +        continue;
> +    }

It seems enough to check to see that the add has a single use, and  
that its use is a phinode in the loop header, no?

Looks like a great start Devang!

-Chris