[llvm-commits] [llvm] r152556 - in /llvm/trunk: lib/Transforms/IPO/Inliner.cpp test/Transforms/Inline/inline_constprop.ll

[Chris Lattner]

On Mar 12, 2012, at 4:19 AM, Chandler Carruth wrote:

> Author: chandlerc
> Date: Mon Mar 12 06:19:33 2012
> New Revision: 152556
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=152556&view=rev
> Log:
> When inlining a function and adding its inner call sites to the
> candidate set for subsequent inlining, try to simplify the arguments to
> the inner call site now that inlining has been performed.
> 
> The goal here is to propagate and fold constants through deeply nested
> call chains. Without doing this, we loose the inliner bonus that should
> be applied because the arguments don't match the exact pattern the cost
> estimator uses.

Hi Chandler,

I like this is in principle, but...

> +static void simplifyCallSiteArguments(const TargetData *TD, CallSite CS) {
> +  // FIXME: It would be nice to avoid this smallvector if RAUW doesn't
> +  // invalidate operand iterators in any cases.

RAUW doesn't invalidate arg_iterator, nor does instruction deletion (assuming you don't delete CS itself).

> +  SmallVector<std::pair<Value *, Value*>, 4> SimplifiedArgs;
> +  for (CallSite::arg_iterator I = CS.arg_begin(), E = CS.arg_end();
> +       I != E; ++I)
> +    if (Instruction *Inst = dyn_cast<Instruction>(*I))
> +      if (Value *SimpleArg = SimplifyInstruction(Inst, TD))
> +        SimplifiedArgs.push_back(std::make_pair(Inst, SimpleArg));
> +  for (unsigned Idx = 0, Size = SimplifiedArgs.size(); Idx != Size; ++Idx)
> +    SimplifiedArgs[Idx].first->replaceAllUsesWith(SimplifiedArgs[Idx].second);
> +}

I can see how this helps your simple example, but wouldn't it fail on something like (note the addition of the *2):

+///   void outer(int x) {
+///     if (x < 42)
+///       return inner(42 - x*2);
+///     ...
+///   }
+///   void inner(int x) {
+///     ...
+///   }

In my opinion, the right fix for this is to prevent the inlined code from being un-constant-folded.  When substituting in the argument into the cloned body, can't we recursively simplify things from the def through the uses, instead of one level up from the use?

-Chris