[PATCH] D33946: [InlineCost] Find identical loads in the callee
Easwaran Raman via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Aug 22 14:25:29 PDT 2017
eraman added a comment.
LGTM with the comments addressed, but please check with Chandler if he has more to add.
================
Comment at: lib/Analysis/InlineCost.cpp:265
unsigned SROACostSavingsLost;
+ unsigned CSELoadCostSavings;
----------------
haicheng wrote:
> mehdi_amini wrote:
> > haicheng wrote:
> > > mehdi_amini wrote:
> > > > chandlerc wrote:
> > > > > Similarly, I would call this `LoadCSECostSavings`.
> > > > Why do we have both `LoadEliminationCostSavings` and `LoadEliminationCost` ?
> > > `LoadEliminationCost` is used in the cost calculation, and `LoadEliminationCostSavings` is used for stats printing. This is the same as `SROACostSavings` and `SROAArgCosts`.
> > I'm still not making sense of the difference between the two: as far as I can tell they are zero-initialized in the same ctor and gets incremented the same way?
> Your understanding is correct. If load elimination is not disabled, they should have the same value. LoadEliminationCostSavings is just used in stats printing.
>
> Eventually, every load address needs to have its own LoadEliminationCost as written in the FIXME no.1. At that time, LoadEliminationCostSavings is the sum of all LoadEliminationCost.
I understand that once you improve this you might mimic what SROA does and need to have a separate variable for both. But you don' need this now and so I agree with Mehdi's comments.
================
Comment at: lib/Analysis/InlineCost.cpp:150
+ bool EnableLoadElimination;
+ SmallPtrSet<Value *, 12> LoadAddrSet;
+ int LoadEliminationCost;
----------------
Nit: SmallPtrSet rounds up the size to a power of 2 and so this will get rounded up to 16. I prefer using a power of 2 explicitly rather than just letting the implementation round it up.
================
Comment at: test/Transforms/Inline/redundant-loads.ll:67
+ call void @foo()
+ %2 = load i32, i32* %a
+ ret void
----------------
Even if this load is eliminated, this callee will not be inlined into the caller (cost and threshold are both 0) and so you're really not testing that the call is preventing the second load from being eliminated. One fix is to increase the threshold to 5 and adjust the other cases slightly if needed.
Repository:
rL LLVM
https://reviews.llvm.org/D33946
More information about the llvm-commits
mailing list