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

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: lib/Analysis/InlineCost.cpp:659
+  // should *always* be considered cold.
+  if (Callee.hasFnAttribute(Attribute::Cold))
+    return true;
----------------
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.


https://reviews.llvm.org/D36722