[PATCH] D36722: [InlineCost] Simplify the cold attribute handling in inline-cost.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added inline comments.


================
Comment at: lib/Analysis/InlineCost.cpp:659
+  // should *always* be considered cold.
+  if (Callee.hasFnAttribute(Attribute::Cold))
+    return true;
----------------
davidxl wrote:
> chandlerc wrote:
> > davidxl wrote:
> > > chandlerc wrote:
> > > > davidxl wrote:
> > > > > chandlerc wrote:
> > > > > > davidxl wrote:
> > > > > > > IIRC, static branch prediction already handles this case. Do you see missing cases?
> > > > > > Sorry, I tried to explain in my top-level description -- there are cases where despite being cold, the call site won't be cold relative to the function entry. For example, when the call site is *in* the function entry block.
> > > > > > 
> > > > > > Maybe this still isn't making sense? I can add an example bit of code.
> > > > > There is an inconsistency between cold callsite relative freq and the cold branch probability --- it is 6% vs 2%. I think we should fix one of them -- IMO, it is more reasonable to fix the static branch prediction.
> > > > > 
> > > > > 
> > > > Sure? I don't have strong feelings about that. But it seems like that is orthogonal? (Or am I missing something?)
> > > > 
> > > > Before this patch, if the branch probability check below failed, we would continue onward and ask PSI and it would still say the call is cold based on this attribute. (I think...)
> > > See my comment to the other patch.  My point is what we should not rely this special hand shaking to do this.  The generic attribute propagation should already kick in to mark certain callers of a cold callee as cold, and the BFI of the caller's caller should be already good enough to detect this.
> > Ok, let's discuss that on that patch.
> > 
> > This patch is *just* trying to cleanup the APIs to make them easier to understand and write tests against. It's just simplifying things and doesn't introduce any significant changes to behavior (that I'm aware of).... Regardless of what we do with the other patch I think this makes the code better?
> What you said makes sense in some way. However it feels wrong. If we allow the special handling of cold attribute here, what prevents other special logics from being duplicated here as well --- for instance, EH code, unreachable code, etc .. ?
All of those have control flow around them which would exhibit the coldness in the BFI analysis, so I don't know why would need them...

The key to me is that it may be useful to change inlining behavior of a cold call even if there is no control flow guarding that callsite.


But all of this seems a discussion around reconsidering the *original* introduction of the logic to check the cold attribute in the `ProfileSummaryInfo` analysis that was added in r271728 which you reviewed in D20648. I'm fine if you want to revisit the decision to check this attribute, but this patch just moves it around, so I don't know why we should block a cleanup on that...


https://reviews.llvm.org/D36722