[PATCH] D37076: [LICM] Allow sinking when foldable in loop

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added inline comments.


================
Comment at: lib/Transforms/Scalar/LICM.cpp:715
+  if (GEP && isa<LoadInst>(UserI) && (I->getParent() == UserI->getParent())) {
+    SmallVector<const Value *, 4> Indices;
+    for (auto I = GEP->idx_begin(); I != GEP->idx_end(); ++I)
----------------
hfinkel wrote:
> hfinkel wrote:
> > This can just be:
> > 
> >   return TTI->getUserCost(GEP) == TargetTransformInfo::TCC_Free;
> > 
> > (this code here to call getGEPCost seems to duplicate the implementation logic of getUserCost)
> > 
> > On that note, you might not even have to restrict this to GEPs used by Loads, but rather, you could allow all zero-cost instructions.
> (Also, I think that using all 'Free' instructions has the benefit of being correlated with the unrolling cost of the loop - these are the instructions that won't increase the unrolling cost of the loop).
Use getUercost() directly, instead of getGEPCost().

It might be possible to allow this for all other zero-cost instructions. However, I'm not perfectly sure if this is good or needed for all other free instructions. For example, I'm not clear if sinking a free trunc is needed? However, in GEP case, by sinking a GEP, we can decouple the users of the GEP: one in loop and one in outside of the loop so that the one in loop will be folded in isel if they are in the same block. 

I think we need extensive tests before opening up this for all other free instruction,s and isolating this for GEP as a first step would make review process easy.   


================
Comment at: lib/Transforms/Scalar/LICM.cpp:715
+  if (GEP && isa<LoadInst>(UserI) && (I->getParent() == UserI->getParent())) {
+    SmallVector<const Value *, 4> Indices;
+    for (auto I = GEP->idx_begin(); I != GEP->idx_end(); ++I)
----------------
junbuml wrote:
> hfinkel wrote:
> > hfinkel wrote:
> > > This can just be:
> > > 
> > >   return TTI->getUserCost(GEP) == TargetTransformInfo::TCC_Free;
> > > 
> > > (this code here to call getGEPCost seems to duplicate the implementation logic of getUserCost)
> > > 
> > > On that note, you might not even have to restrict this to GEPs used by Loads, but rather, you could allow all zero-cost instructions.
> > (Also, I think that using all 'Free' instructions has the benefit of being correlated with the unrolling cost of the loop - these are the instructions that won't increase the unrolling cost of the loop).
> Use getUercost() directly, instead of getGEPCost().
> 
> It might be possible to allow this for all other zero-cost instructions. However, I'm not perfectly sure if this is good or needed for all other free instructions. For example, I'm not clear if sinking a free trunc is needed? However, in GEP case, by sinking a GEP, we can decouple the users of the GEP: one in loop and one in outside of the loop so that the one in loop will be folded in isel if they are in the same block. 
> 
> I think we need extensive tests before opening up this for all other free instruction,s and isolating this for GEP as a first step would make review process easy.   
I don't think Free from getUserCost guarantee that the instruction is folded away always.  So, I specifically check for a GEP which could be  a legal addressing mode and it's used in a load / store in the same block, expecting the isel fold them into its users.
 


https://reviews.llvm.org/D37076