[PATCH] D27696: [ThinLTO] Thin link efficiency: skip candidate added later with higher threshold (NFC)

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:330
+    bool IsHotCallsite = Edge.second.Hotness == CalleeInfo::HotnessType::Hot;
+    const auto AdjThreshold = GetAdjustedThreshold(Threshold, IsHotCallsite);
+
----------------
mehdi_amini wrote:
> tejohnson wrote:
> > mehdi_amini wrote:
> > > mehdi_amini wrote:
> > > > mehdi_amini wrote:
> > > > > mehdi_amini wrote:
> > > > > > I'm fairly lost in the logic here between `GetAdjustedThreshold` and `GetBonusMultiplier`, still trying to make sense of what we're currently doing here.
> > > > > OK I figured what's going on, it is quite obscure to infer from the code though, and I believe the comment above is misleading.
> > > > (I take back what I wrote for the comment, was looking at the wrong place... So confusing!)
> > > This code motion to use `AdjThreshold` instead of `Threshold` below seems like a standalone fix?
> > > I'm fairly lost in the logic here between GetAdjustedThreshold and GetBonusMultiplier, still trying to make sense 
> > > of what we're currently doing here.
> > 
> > The difference is that GetBonusMultiplier is only applied to the current callsite, when the call is hot. It isn't recorded or passed down to the next level of calls (otherwise it would be compounded). GetAdjustedThreshold applies the decay factor for the next level of calls.
> > 
> > > This code motion to use AdjThreshold instead of Threshold below seems like a standalone fix?
> > 
> > This isn't a fix per se. By itself this part of the change should be a no op.
> > 
> > E.g. before we would record the (non-decayed) Threshold in the ImportLists, which would then be pulled out into ProcessedThreshold and compared again to the (non-decayed) Threshold here the next time we saw this function.
> > With this change, we instead record the decayed AdjThreshold in the ImportLists, and compare against the new decayed AdjThreshold here the next time we see this function.
> > 
> > The reason for this change is so that we can compare the decayed threshold recorded in the Worklist to the threshold recorded in the ImportLists further down when we iterate through the Worklist. I.e. we need to compare apples to apples there.
> > The difference is that GetBonusMultiplier is only applied to the current callsite, when the call is hot. It isn't recorded or passed down to the next level of calls (otherwise it would be compounded). GetAdjustedThreshold applies the decay factor for the next level of calls.
> 
> Yeah I got it in the end, still not straightfoward from the code. I though about improving but can't figure :)
> A high level description may be helpful, I'll think about it.
> 
> > This isn't a fix per se. By itself this part of the change should be a no op.
> >
> > E.g. before we would record the (non-decayed) Threshold in the ImportLists, which would then be pulled out into > ProcessedThreshold and compared again to the (non-decayed) Threshold here the next time we saw this function.
> > With this change, we instead record the decayed AdjThreshold in the ImportLists, and compare against the new decayed AdjThreshold here the next time we see this function.
> 
> I was considering the case where a function can be reached from a hot path or from a call path, and it didn't seem like a NFC change. For instance, 
> 
> 1) first visit with a cold edge and a threshold of 100 -> set ProcessedThreshold to 100 and push the decayed threshold, let say 70, on the stack. 
> 2) visit with a hot edge and a threshold of 99 -> compare to the previous ProcessedThreshold and decide to not push on the stack.
> 
> With your change, I believe we would:
> 
> 1) first visit with the cold edge with a threshold of 100 -> set ProcessedThreshold to the decayed threshold of 70 and push it on the stack. 
> 2) visit with the hot edge and a threshold of 99 -> compare to the previous ProcessedThreshold and decide to set ProcessedThreshold to the decayed threshold of 99 and push it on the stack.
> 
Gah - you're right! This change indeed fixes a bug in the logic, where we could have been missing imports along some hot paths.  I will come up with a test case that is affected by this change. 

Do you want me to split this into a different patch on Phab, or is it enough to commit separately with a test case?

Regarding the confusing handling of different bonuses, let me add a comment where we compute these two different adjustments.


https://reviews.llvm.org/D27696