[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:
> 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.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:343
     // Mark this function as imported in this module, with the current Threshold
-    ProcessedThreshold = Threshold;
+    ProcessedThreshold = AdjThreshold;
 
----------------
mehdi_amini wrote:
> All this logic to skip inserting in the worklist seems redundant with the new logic you added below while iterating over the work list (the latter seems to be a superset).
> 
>  
> All this logic to skip inserting in the worklist seems redundant with the new logic you added
> below while iterating over the work list (the latter seems to be a superset).

It's true that with the change I am adding below where we iterate over the work list this is no longer strictly necessary. However, there's no good reason to insert into the work list again with a lower threshold if we know we have already added this function with a higher threshold, it will just make the work list longer for no benefit. And will require a redundant add of this GUID to the ExportList below - all easily avoidable since we have to access the current threshold anyway to update it above.

The handling I added below to skip work list items when we pull them off the work list is to handle the case where we later added the function at a higher threshold (and it was already in the work list at the earlier lower threshold).


https://reviews.llvm.org/D27696