[PATCH] D35823: [Inliner] Do not apply any bonus for cold callsites.

[Easwaran Raman via Phabricator via llvm-commits]

eraman added a comment.

In https://reviews.llvm.org/D35823#819607, @chandlerc wrote:

>




> Does function-attrs mark these as coldcc so that we use a cheaper calling convention for them?

No. In InlineCost, calls with coldcc get a high penalty (2000). So marking them as coldcc in function-attrs (which comes before inlining) will have the effect of not inlining them.

> If not, we should do that. We should also discount the cost of calling them when computing other functions' inline cost.

It's not clear to me why we should do this. Is it because coldcc calls are cheaper (or even free)?
 >Probably just worth a FIXME comment here or a mention in the description as future work.



================
Comment at: lib/Analysis/InlineCost.cpp:712
           DEBUG(dbgs() << "Cold callsite.\n");
+          DoNotApplyBonuses = true;
           Threshold = MinIfValid(Threshold, Params.ColdCallSiteThreshold);
----------------
chandlerc wrote:
> Would it make more sense to just update the bonuses to reflect what they should be for cold call sites?
> 
> I mostly find the boolean interaction with all the bonuses very brittle and easy got get wrong. Are there other ways of structuring this?
> 
> If not, I might set the boolean based on semantics rather than logic -- specifically that this is a *cold* call edge. That will make it more obvious why we would skip bonuses below for that case.
I've attempted to do this by keeping the bonus percentages as variables that are reset when the callsite is cold. 


================
Comment at: lib/Analysis/InlineCost.cpp:1328-1329
       F.hasLocalLinkage() && F.hasOneUse() && &F == CS.getCalledFunction();
-  if (OnlyOneCallAndLocalLinkage)
+  if (OnlyOneCallAndLocalLinkage && !DoNotApplyBonuses)
     Cost -= InlineConstants::LastCallToStaticBonus;
 
----------------
chandlerc wrote:
> However you end up changing around the logic above, you should update the comments here to make it *really clear* what tradeoff you're making with this code as it is different now.
> 
> Notably, we have a bool whose only purpose is to organize the predicates going into this if, and now we have a new predicate that is not in the bool... which I think makes sense, but is a little odd to be omitted from the comment.
I've added comments saying why we disable this even if it might reduce code size. Let me know if you are expecting something more.


https://reviews.llvm.org/D35823