[PATCH] D33946: [InlineCost] Find identical loads in the callee

[Easwaran Raman via Phabricator via llvm-commits]

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