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

[Chris Lattner]

> Add FastDSE, a new algorithm for doing dead store elimination.   
> This algorithm is not as accurate
> as the current DSE, but it only a linear scan over each block,  
> rather than quadratic.  Eventually
> (once it has been improved somewhat), this will replace the current  
> DSE.

Woot.


> +//===- DeadStoreElimination.cpp - Dead Store Elimination  
> ------------------===//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file was developed by the LLVM research group and is  
> distributed under

s/LLVM research group/the amazing and unpredictable owen/

> +FunctionPass *llvm::createFastDeadStoreEliminationPass() { return  
> new FDSE(); }
> +
> +bool FDSE::runOnBasicBlock(BasicBlock &BB) {
> +  MemoryDependenceAnalysis& MD =  
> getAnalysis<MemoryDependenceAnalysis>();
> +
> +  DenseMap<Value*, StoreInst*> lastStore;
> +  SetVector<Instruction*> possiblyDead;

Please add comments describing what these are and what they contain  
at any point in time.

> +  bool MadeChange = false;
> +
> +  // Do a top-down walk on the BB
> +  for (BasicBlock::iterator BBI = BB.begin(), BBE = BB.end(); BBI ! 
> = BBE; ++BBI) {
> +    // If we find a store...
> +    if (StoreInst* S = dyn_cast<StoreInst>(BBI)) {

Since you only look at stores, I suggest structuring this loop like  
this to reduce nesting (which makes it easier to read the code):

> +  for (BasicBlock::iterator BBI = BB.begin(), BBE = BB.end(); BBI ! 
> = BBE; ++BBI) {
> +    // If we find a store...
> +    StoreInst* S = dyn_cast<StoreInst>(BBI);
> +    if (!S) continue;
...



> +
> +      // ... to a pointer that has been stored to before...
> +      if (lastStore.count(S->getPointerOperand())) {
> +        StoreInst* last = lastStore[S->getPointerOperand()];

This does two hashtable lookups, and the insert below does another.   
I'd suggest just doing:

     StoreInst *&last = lastStore[S->getPointerOperand()];
     if (last) {
        ...
     }
     last = S;

> +        // ... and no other memory dependencies are between them....
> +        if (MD.getDependency(S) == last) {
> +          // Remove it!
> +          MD.removeInstruction(last);
> +
> +          // DCE instructions only used to calculate that store
> +          if (Instruction* D = dyn_cast<Instruction>(last- 
> >getOperand(0)))
> +            possiblyDead.insert(D);
> +
> +          last->eraseFromParent();
> +          NumFastStores++;
> +          MadeChange = true;
> +        }
> +      }

Nice and simple!

> +      // Update our most-recent-store map
> +      lastStore.insert(std::make_pair(S->getPointerOperand(), S));
> +    }
> +  }
> +
> +  // Do a trivial DCE
> +  while (!possiblyDead.empty()) {
> +    Instruction *I = possiblyDead.back();
> +    possiblyDead.pop_back();
> +    DeleteDeadInstructionChains(I, possiblyDead);
> +  }
> +
> +  return MadeChange;
> +}
> +
> +void FDSE::DeleteDeadInstructionChains(Instruction *I,
> +                                      SetVector<Instruction*>  
> &DeadInsts) {
> +  // Instruction must be dead.
> +  if (!I->use_empty() || !isInstructionTriviallyDead(I)) return;
> +
> +  // Let the memory dependence know
> +  getAnalysis<MemoryDependenceAnalysis>().removeInstruction(I);
> +
> +  // See if this made any operands dead.  We do it this way in  
> case the
> +  // instruction uses the same operand twice.  We don't want to  
> delete a
> +  // value then reference it.
> +  for (unsigned i = 0, e = I->getNumOperands(); i != e; ++i) {
> +    if (Instruction *Op = dyn_cast<Instruction>(I->getOperand(i)))
> +      DeadInsts.insert(Op);      // Attempt to nuke it later.
> +    I->setOperand(i, 0);         // Drop from the operand list.

With this design, there is no reason to call setOperand.  You  
probably want something like this:

> +  for (unsigned i = 0, e = I->getNumOperands(); i != e; ++i) {
 > +    if (I->getOperand(i)->hasOneUse())
> +      if (Instruction *Op = dyn_cast<Instruction>(I->getOperand(i)))
> +        DeadInsts.insert(Op);      // Attempt to nuke it later.
> +    I->setOperand(i, 0);         // Drop from the operand list.

Otherwise, nice work!  How fast is this on the slow bugzilla case  
compared to slow dse?

-Chris


> +  }
> +
> +  I->eraseFromParent();
> +  ++NumFastOther;
> +}
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits