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

[Hal Finkel via Phabricator via llvm-commits]

hfinkel 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)
----------------
junbuml wrote:
> 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.
>  
> I don't think Free from getUserCost guarantee that the instruction is folded away always.

Yes, it should be. Free means free. If not, then there's something wrong with the cost model that we should fix.

> 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 don't understand how the advantages, or disadvantages, of doing this for a free truncate are different from a free GEP. In both cases, we decouple things inside the loop from outside the loop allowing the folding to take place later.

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

Then please run tests (I assume you ran tests for this, as proposed, too). This makes the review harder. LICM is part of our canonicalization process, and we need to have an understandable canonical form. The more that this turns into a patchwork of heuristics, the harder it is to figure out what our canonical form is. "We always decouple free instructions" is easy to explain. We sometimes decouple GEPs if they happen to be used in certain ways is harder.


https://reviews.llvm.org/D37076