[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 30, 2010, at 4:34 PM, Owen Anderson wrote:
> On Aug 30, 2010, at 3:46 PM, Chris Lattner wrote:
>>> +++ 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.
> 
> I don't really have a better idea for it.  I'm open to suggestions.

How about "correlated value prop" or something like that?

>>> +  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?
> 
> The actual value propagation will tend to leave a lot of unused values and/or newly-constant PHI nodes sitting around.  These can't be removed/simplified during the main loop without invalidating the BasicBlock::iterator.

Why not leave it to a later pass to clean up?  Why not clean up each basic block after you walk if if any changes were made in that block?

> If ValuePropagation is run early enough, this probably isn't necessary as later runs of instcombine should clean up after it.  However, if we want to run it reasonably late, we probably want it to clean up after itself.

What's the right answer?

-Chris