[llvm-commits] [llvm] r119853 - /llvm/trunk/lib/Transforms/Scalar/CodeGenPrepare.cpp

Duncan Sands baldrick at free.fr
Fri Nov 19 23:16:56 PST 2010


Hi Owen,

> When folding addressing modes in CodeGenPrepare, attempt to look through PHI nodes
> if all the operands of the PHI are equivalent.  This allows CodeGenPrepare to undo
> unprofitable PRE transforms.

is PRE done at the codegen level?  If not, instcombine should have cleaned this
up already.

> --- llvm/trunk/lib/Transforms/Scalar/CodeGenPrepare.cpp (original)
> +++ llvm/trunk/lib/Transforms/Scalar/CodeGenPrepare.cpp Fri Nov 19 16:15:03 2010
> @@ -618,6 +618,32 @@
>   bool CodeGenPrepare::OptimizeMemoryInst(Instruction *MemoryInst, Value *Addr,
>                                           const Type *AccessTy,
>                                           DenseMap<Value*,Value*>  &SunkAddrs) {
> +  // Try to collapse single-value PHI nodes.  This is necessary to undo
> +  // unprofitable PRE transformations.
> +  Value *Repl = Addr;
> +  if (isa<PHINode>(Addr)&&  MemoryInst->hasOneUse()) {
> +    PHINode *P = cast<PHINode>(Addr);
> +    Instruction *Consensus = 0;
> +    unsigned NumUses = 0;

What is NumUses for?  You seem to use it to choose the operand with the
most uses, but why?

> +    for (unsigned i = 0, e = P->getNumIncomingValues(); i != e; ++i) {
> +      Instruction *Incoming = dyn_cast<Instruction>(P->getIncomingValue(i));

If all incoming values are constants, and equal, isn't that a good thing, i.e.
you should replace the phi with the constant?

> +      if (!Incoming || (Consensus&&  !Incoming->isIdenticalTo(Consensus))) {

How do you ensure that Consensus dominates the phi node?  If the instructions
really were all identical, then it is automatic that the common value dominates
the phi node (and thus that it is safe to use it to replace the phi), but since
you use isIdentical to I don't see why this is guaranteed.

> +        Consensus = 0;
> +        break;
> +      }
> +
> +      if (!Consensus || Incoming->isIdenticalTo(Consensus)) {
> +        if (Incoming->getNumUses()>  NumUses) {
> +          Consensus = Incoming;
> +          NumUses = Incoming->getNumUses();
> +        }
> +        continue;
> +      }
> +    }
> +
> +    if (Consensus) Addr = Consensus;

Shouldn't this be "Repl = Consensus"?

Also - got a testcase?

Ciao,

Duncan.



More information about the llvm-commits mailing list