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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

This change makes lots of sense to me.... However, the change description should really spell out more details about what's going on. You should assume that some of the readers of the change description don't know (or have forgotten) some of the context in your head about why this is all important and how it fits together.

I'm guessing that far and away the biggest difference is the last-call bonus? I suspect it is worth calling out that specific case.

Does function-attrs mark these as coldcc so that we use a cheaper calling convention for them? If not, we should do that. We should also discount the cost of calling them when computing other functions' inline cost. 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);
----------------
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.


================
Comment at: lib/Analysis/InlineCost.cpp:1328-1329
       F.hasLocalLinkage() && F.hasOneUse() && &F == CS.getCalledFunction();
-  if (OnlyOneCallAndLocalLinkage)
+  if (OnlyOneCallAndLocalLinkage && !DoNotApplyBonuses)
     Cost -= InlineConstants::LastCallToStaticBonus;
 
----------------
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.


https://reviews.llvm.org/D35823