[llvm-commits] [llvm] r112325 - in /llvm/trunk: include/llvm/LinkAllPasses.h include/llvm/Transforms/Scalar.h lib/Transforms/Scalar/ValuePropagation.cpp test/Transforms/ValuePropagation/ test/Transforms/ValuePropagation/dg.exp test/Transforms/ValuePropagation/phi.ll test/Transforms/ValuePropagation/select.ll

[Chris Lattner]

On Aug 27, 2010, at 4:31 PM, Owen Anderson wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=112325&view=rev
> Log:
> Add a prototype of a new peephole optimizing pass that uses LazyValue info to simplify PHIs and select's.
> This pass addresses the missed optimizations from PR2581 and PR4420.

nifty,

> +++ llvm/trunk/lib/Transforms/Scalar/ValuePropagation.cpp Fri Aug 27 18:31:36 2010
> @@ -0,0 +1,113 @@
> +//===- ValuePropagation.cpp - Propagate information derived control flow --===//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open Source
> +// License. See LICENSE.TXT for details.
> +//
> +//===----------------------------------------------------------------------===//
> +//
> +// This file implements the Value Propagation pass.

The name of this pass is very generic.  Can you think of something better?  It's not really propagating values afterall.

> +bool ValuePropagation::processSelect(SelectInst *S) {
> +  Constant *C = LVI->getConstant(S->getOperand(0), S->getParent());
> +  if (!C) return false;

Does LVI work on vectors?  If not, you should check that the operand of the select is an integer.

> +  if (CI->isZero()) {
> +    S->replaceAllUsesWith(S->getOperand(2));
> +    S->eraseFromParent();
> +  } else if (CI->isOne()) {
> +    S->replaceAllUsesWith(S->getOperand(1));
> +    S->eraseFromParent();
> +  } else {
> +    assert(0 && "Select on constant is neither 0 nor 1?");
> +  }

Instead of three-way, this should be two way:

> +  if (CI->isZero()) {
> +    S->replaceAllUsesWith(S->getOperand(2));
> +    S->eraseFromParent();
> +  } else {
> +    assert(CI->isOne() && "Select on constant isn't a bool?"););
> +    S->replaceAllUsesWith(S->getOperand(1));
> +    S->eraseFromParent();
> +  }


Better yet, use:

  S->replaceAllUsesWith(S->getOperand(CI->isOne() ? 1 : 2));
  S->eraseFromParent();

Redundancy is bad.


> +bool ValuePropagation::processPHI(PHINode *P) {
> +  bool changed = false;

Please capitalize Changed.

> +  
> +  BasicBlock *BB = P->getParent();
> +  for (unsigned i = 0; i < P->getNumIncomingValues(); ++i) {

Don't evaluate P->getNumIncomingValues() every iteration.

> +    Constant *C = LVI->getConstantOnEdge(P->getIncomingValue(i),
> +                                         P->getIncomingBlock(i),
> +                                         BB);
> +    if (!C || C == P->getIncomingValue(i)) continue;

This shouldn't even do the query if the incoming value is already a constant.

> +    
> +    P->setIncomingValue(i, C);
> +    changed = true;
> +  }
> +  
> +  if (Value *ConstVal = P->hasConstantValue()) {
> +    P->replaceAllUsesWith(ConstVal);
> +    P->eraseFromParent();
> +    changed = true;
> +  }
> +  
> +  return changed;
> +}
> +
> +bool ValuePropagation::runOnFunction(Function &F) {
> +  LVI = &getAnalysis<LazyValueInfo>();
> +  
> +  bool changed = false;
> +  
> +  for (Function::iterator FI = F.begin(), FE = F.end(); FI != FE; ++FI)
> +    for (BasicBlock::iterator BI = FI->begin(), BE = FI->end(); BI != BE; ) {
> +      Instruction *II = BI++;
> +      if (SelectInst *SI = dyn_cast<SelectInst>(II))
> +        changed |= processSelect(SI);
> +      else if (PHINode *P = dyn_cast<PHINode>(II))
> +        changed |= processPHI(P);
> +    }
> +  
> +  if (changed)
> +    for (Function::iterator FI = F.begin(), FE = F.end(); FI != FE; ++FI)
> +      SimplifyInstructionsInBlock(FI);

This loop is very odd... what does it do and why?  Why does it belong here?

> 
> +; CHECK: ret i32 10
> +        ret i32 10
> +}
> \ No newline at end of file

Please beware that your editor is not adding newlines at the end of files and try to fix it.

-Chris

> 
> Added: llvm/trunk/test/Transforms/ValuePropagation/select.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/ValuePropagation/select.ll?rev=112325&view=auto
> ==============================================================================
> --- llvm/trunk/test/Transforms/ValuePropagation/select.ll (added)
> +++ llvm/trunk/test/Transforms/ValuePropagation/select.ll Fri Aug 27 18:31:36 2010

Why make each of these their own file?  This should go into a basic.ll which tests basic functionality of the pass.

-Chris
 

> @@ -0,0 +1,25 @@
> +; RUN: opt < %s -value-propagation -S | FileCheck %s
> +; PR4420
> +
> +declare i1 @ext()
> +; CHECK: @foo
> +define i1 @foo() {
> +entry:
> +        %cond = tail call i1 @ext()             ; <i1> [#uses=2]
> +        br i1 %cond, label %bb1, label %bb2
> +
> +bb1:            ; preds = %entry
> +        %cond2 = tail call i1 @ext()            ; <i1> [#uses=1]
> +        br i1 %cond2, label %bb3, label %bb2
> +
> +bb2:            ; preds = %bb1, %entry
> +; CHECK-NOT: phi i1
> +        %cond_merge = phi i1 [ %cond, %entry ], [ false, %bb1 ]         ; <i1> [#uses=1]
> +; CHECK: ret i1 false
> +        ret i1 %cond_merge
> +
> +bb3:            ; preds = %bb1
> +        %res = tail call i1 @ext()              ; <i1> [#uses=1]
> +; CHECK: ret i1 %res
> +        ret i1 %res
> +}
> \ No newline at end of file
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits