Chang sign of LastCallToStaticBouns

[Prazek]

Summary

I think it is much better this way.
When I firstly saw line:

Cost += InlineConstants::LastCallToStaticBonus;

I though that this is a bug, because everywhere where the cost is being reduced
it is usuing -=.

[mehdi_amini]

include/llvm/Analysis/InlineCost.h

37 ↗

(On Diff #67006)

It made more sense to me before: now "Bonus" and "Penalty" are both positive.

lib/Analysis/InlineCost.cpp

1266 ↗

(On Diff #67006)

Here is also +=, I feel you're creating some inconsistency

[eraman]

lib/Analysis/InlineCost.cpp

1261 ↗

(On Diff #67006)

This is not the only use of this constant - it is used in Inliner.cpp also. It also feels a bit odd to subtract a "bonus", but I have no strong opinions.

[Prazek]

I don't think it is inconsistent.
The name "Bonus" suggest that it should be substracted to lower the cost, the name Penality suggest that it should be added so the cost rise.

[Prazek]

Update missing inst

[Prazek]

lib/Analysis/InlineCost.cpp

1261 ↗

(On Diff #67012)

Thanks, I grepped only this file, and llvm tests also passed...

[Prazek]

So IMHO it is better to express increase of cost by add and decrease by substract, than mixing both.

[Prazek]

lib/Analysis/InlineCost.cpp

1266 ↗

(On Diff #67012)

But right now it is consistent with line 1248 and 1252

[Prazek]

ping Mehdi

[mehdi_amini]

I'm not totally convinced, but if someone else thinks it is a good idea I won't oppose either.

[tejohnson]

I agree with Piotr - to me it seems more intuitive to subtract a bonus from the "Cost" and to add the penalty. I have to admit I had the same confusion the first time I looked at this code, until I discovered that the Bonus was negative.

[hans]

lgtm, seems like an obvious improvement to me.

[Prazek]

rebase

llvm/trunk/include/llvm/Analysis/InlineCost.h

 
 // Use when -O3 is specified.
 const int OptAggressiveThreshold = 275;
 
 // Various magic constants used to adjust heuristics.
 const int InstrCost = 5;
 const int IndirectCallThreshold = 100;
 const int CallPenalty = 25;
-const int LastCallToStaticBonus = -15000;
+const int LastCallToStaticBonus = 15000;
 const int ColdccPenalty = 2000;
 const int NoreturnPenalty = 10000;
 /// Do not inline functions which allocate this many bytes on the stack
 /// when the caller is recursive.
 const unsigned TotalAllocaSizeRecursiveCaller = 1024;
 }
 
 /// \brief Represents the cost of inlining a function.

llvm/trunk/lib/Analysis/InlineCost.cpp

for (unsigned I = 0, E = CS.arg_size(); I != E; ++I) {
     }
   }
 
   // If there is only one call of the function, and it has internal linkage,
   // the cost of inlining it drops dramatically.
   bool OnlyOneCallAndLocalLinkage =
       F.hasLocalLinkage() && F.hasOneUse() && &F == CS.getCalledFunction();
   if (OnlyOneCallAndLocalLinkage)
-    Cost += InlineConstants::LastCallToStaticBonus;
+    Cost -= InlineConstants::LastCallToStaticBonus;
 
   // If this function uses the coldcc calling convention, prefer not to inline
   // it.
   if (F.getCallingConv() == CallingConv::Cold)
     Cost += InlineConstants::ColdccPenalty;
 
   // Check if we're done. This can happen due to bonuses and penalties.
   if (Cost > Threshold)

llvm/trunk/lib/Transforms/IPO/Inliner.cpp

if (IC2.getCostDelta() <= CandidateCost) {
       TotalSecondaryCost += IC2.getCost();
     }
   }
   // If all outer calls to Caller would get inlined, the cost for the last
   // one is set very low by getInlineCost, in anticipation that Caller will
   // be removed entirely.  We did not account for this above unless there
   // is only one caller of Caller.
   if (callerWillBeRemoved && !Caller->use_empty())
-    TotalSecondaryCost += InlineConstants::LastCallToStaticBonus;
+    TotalSecondaryCost -= InlineConstants::LastCallToStaticBonus;
 
   if (inliningPreventsSomeOuterInline && TotalSecondaryCost < IC.getCost())
     return true;
 
   return false;
 }
 
 /// Return true if the inliner should attempt to inline at the given CallSite.