[llvm-commits] PATCH: Major rewrite of inline cost analysis.

[Duncan Sands]

Hi Chandler,

> +    InlineCost getInlineCost(CallSite CS, int Threshold);
>
>      /// resetCachedFunctionInfo - erase any cached cost info for this function.
>      void resetCachedCostInfo(Function* Caller) {
> -      CachedFunctionInfo[Caller] = FunctionInfo();
>      }

is the function resetCachedFunctionInfo dead?  It doesn't seem to be doing much
any more :)

> -      // We will get to remove this instruction...
> -      Reduction += InlineConstants::InstrCost;
> +  // Keep a bunch of stats about the cost savings found so we can print them
> +  // out when debugging.
> +  unsigned NumConstantArgs;
> +  unsigned NumConstantOffsetPtrArgs;
> +  unsigned NumAllocaArgs;
> +  unsigned NumConstantPtrCmps;
> +  unsigned NumConstantPtrDiffs;
> +  unsigned NumInstructionsSimplified;
> +  unsigned SROACostSavings;
> +  unsigned SROACostSavingsLost;

Needless to say you should probably use LLVM's statistics infrastructure for
this.

> +/// \brief Check whether a GEP's indices are all constant.
> +///
> +/// Respects any simlified values known during the analysis of this callsite.

simlified -> simplified

> +bool CallAnalyzer::isGEPOffsetConstant(GetElementPtrInst &GEP) {

+bool CallAnalyzer::visitPtrToInt(PtrToIntInst &I) {
+  // Propagate constants through ptrtoint.
+  if (Constant *COp = dyn_cast<Constant>(I.getOperand(0)))
+    if (Constant *C = ConstantExpr::getPtrToInt(COp, I.getType())) {
+      SimplifiedValues[&I] = C;
+      return true;
+    }

-  SmallVector<Value *, 4> Worklist;
-  Worklist.push_back(V);
-  do {
-    Value *V = Worklist.pop_back_val();
-    for (Value::use_iterator UI = V->use_begin(), E = V->use_end();
-         UI != E; ++UI){
-      Instruction *I = cast<Instruction>(*UI);
+  // Track base/offset pairs when converted to a plain integer.
+  std::pair<Value *, APInt> BaseAndOffset
+    = ConstantOffsetPtrs.lookup(I.getOperand(0));
+  if (BaseAndOffset.first)
+    ConstantOffsetPtrs[&I] = BaseAndOffset;

A ptrtoint cast need not be to an integer type with pointer width, i.e. it may
contain an implicit truncate or extend.  Don't you need to adjust for this in
your computation?  Also, is a truncating/extending ptrtoint really free?

Likewise for inttoptr.

> +bool CallAnalyzer::visitICmp(ICmpInst &I) {
> +  Value *LHS = I.getOperand(0), *RHS = I.getOperand(1);
> +  // First try to handle simplified comparisons.
> +  if (!isa<Constant>(LHS))
> +    if (Constant *SimpleLHS = SimplifiedValues.lookup(LHS))
> +      LHS = SimpleLHS;
> +  if (!isa<Constant>(RHS))
> +    if (Constant *SimpleRHS = SimplifiedValues.lookup(RHS))
> +      RHS = SimpleRHS;
> +  if (Constant *CLHS = dyn_cast<Constant>(LHS))
> +    if (Constant *CRHS = dyn_cast<Constant>(RHS))
> +      if (Constant *C = ConstantExpr::getICmp(I.getPredicate(), CLHS, CRHS)) {

There are a couple of places like this where you only do constant folding, but
why not do this and more using InstSimplify?

> +        SimplifiedValues[&I] = C;
> +        return true;
>        }

Also, there's a bunch of logic that should probably be sunk into instsimplify
someday, presumably by passing the instsimplify routines some kind of operand
remapping object.  But I guess you know that :)

> +// FIXME: Switch to callsite.
> +bool CallAnalyzer::visitCall(CallInst &I) {

Do you handle invokes?

> +  if (Function *F = CS.getCalledFunction()) {
> +    if (F == I.getParent()->getParent()) {
> +      // This flag will fully abort the analysis, so don't bother with anything
> +      // else.
> +      IsRecursive = true;
> +      return false;
> +    }

Recursive inlining may sometimes be useful...  But I guess you inherited this,
along some rather dubious bonuses etc I noticed.

> +/// growCachedCostInfo - update the cached cost info for Caller after Callee has
> +/// been inlined.
> +void
> +InlineCostAnalyzer::growCachedCostInfo(Function *Caller, Function *Callee) {
>  }

This also seems to be not very useful...

In spite of these nitpicks, I agree with Benjamin that the sooner you get this
in the better.  I ran it on the dragonegg testsuite for a few hours and didn't
see any crashes or infinite loops, so I guess that is something :)

Ciao, Duncan.