Use updated threshold for indirect call bonus

[eraman]

Summary

When considering foo->bar inlining, if there is an indirect call in foo which gets resolved to a direct call (say baz), then we try to inline baz into bar with a threshold T and subtract max(T - Cost(bar->baz), 0) from Cost(foo->bar). This patch uses max(Threshold(bar->baz) - Cost(bar->baz)) instead, where Threshold(bar->baz) could be different from T due to bonuses or subtractions. This seems more logical to me since Threshold(bar->baz) - Cost(bar->baz) represents the desirability of inlining baz into bar than T - Cost(bar->baz), 0.

[davidxl]

yes, this makes more sense to me.

Conceptually, CA.getThreshold() represents the cost if the call site remains *uninlined*, so that Threshold - Cost really means the benefit/gain of doing the inlining.

However since the cost of keeping and indirect call site 'uninlined' can be slightly higher than the regular direct callsite (due to indirect branch), it might be better to introduce a small booster InlineConstants::IndirectCallCost:

Cost -= std::max(0, CA.getThreshold() + InlineConstants::IndirectCallCost - CA.getCost());

Also if InlineConstants::IndirectCallThreshold is not used anywhere, get rid of it.

[eraman]

Ping

[davidxl]

I had a comment there -- remove the IndirectCallThreshold if not needed. The boost parameter suggestion can be done as a followup.

[eraman]

Also if InlineConstants::IndirectCallThreshold is not used anywhere, get rid of it.

It is being used in creating the CallAnalyzer object - this is the threshold we want to begin with.

[chandlerc]

While the source code change here makes sense, there is no test added which is not acceptable, and there was no explicit LGTM.

David, just marking a patch as accepted in Phabricator doesn't send an email. Please actually write an LGTM in the comment box and send that so others are aware.

Also, David, I would suggest waiting for others to give a final approval for changes to the inline cost mechanisms, as this isn't really an area of LLVM that you've done extensive work on (yet). Also, it isn't appropriate to LGTM this without a test that ensures we don't regress.

Easwaran, please add a test.

[chandlerc]

In D14309#304650, @chandlerc wrote:

David, just marking a patch as accepted in Phabricator doesn't send an email. Please actually write an LGTM in the comment box and send that so others are aware.

I've also filed a bug to track the fact that realistically we shouldn't need to remember this kind of stuff. The tools should just DTRT. http://llvm.org/PR25771

[davidxl]

llvm/trunk/lib/Analysis/InlineCost.cpp

834

I missed this unchanged line.

On second look at the patch, I think the patch is still not complete. We should not pass a fixed Threshold for indirect callsite here -- there are other factors that can boost the inlining for the callsite once it becomes direct -- think about PGO case where the indirect callsite is super hot.

Instead, as I said in previous comments, we should make IndirectCallThreshold as an additional booster (delta) on top of regular threshold computed.

[davidxl]

The refactoring should be straight forward -- introduce a new function Inliner::getInlineThreshold that takes two arguments: 1) callsite; and 2) callee function decl.

Can you please help with this?

[chandlerc]

FWIW, this should really be in a separate patch rather than being combined
IMO.

llvm/trunk/lib/Analysis/InlineCost.cpp

bool CallAnalyzer::visitCallSite(CallSite CS) {
   if (!F)
     return Base::visitCallSite(CS);
 
   // If we have a constant that we are calling as a function, we can peer
   // through it and see the function target. This happens not infrequently
   // during devirtualization and so we want to give it a hefty bonus for
   // inlining, but cap that bonus in the event that inlining wouldn't pan
   // out. Pretend to inline the function, with a custom threshold.
   CallAnalyzer CA(TTI, ACT, *F, InlineConstants::IndirectCallThreshold, CS);
   if (CA.analyzeCall(CS)) {
     // We were able to inline the indirect call! Subtract the cost from the
-    // bonus we want to apply, but don't go below zero.
-    Cost -= std::max(0, InlineConstants::IndirectCallThreshold - CA.getCost());
+    // threshold to get the bonus we want to apply, but don't go below zero.
+    Cost -= std::max(0, CA.getThreshold() - CA.getCost());
   }
 
   return Base::visitCallSite(CS);
 }
 
 bool CallAnalyzer::visitReturnInst(ReturnInst &RI) {
   // At least one return instruction will be free after inlining.
   bool Free = !HasReturn;