[PATCH] D52671: [InstCombine] Skip merging non-free GEP

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:1681
+    if (isa<Instruction>(Src) && !Src->hasOneUse() &&
+        TTI.getUserCost(Src) != TargetTransformInfo::TCC_Free) {
+      BasicBlock *CurBB = GEP.getParent();
----------------
efriedma wrote:
> junbuml wrote:
> > efriedma wrote:
> > > For array GEPs, we have a check to try to make sure we aren't making a GEP more expensive (see EndsWithSequential etc.)  Maybe instead of adding a different kind of check, it would make more sense to extend the existing check to struct GEPs?
> > Did you mean to move this whole check (line 1680~1687) into EndsWithSequential? 
> No.
> 
> We currently have a check in the "if (EndsWithSequential)" which restricts GEP folding.  That doesn't apply to your testcase because the last index is a struct index.  Instead, it falls into the "else if (isa<Constant>(*GEP.idx_begin())" case, which doesn't have a profitability check.  My suggestion is to adapt the EndsWithSequential profitability check to also apply to struct GEPs.
The last index (%v) in the test testcase is still a pointer of struct, which is sequential. This testcase actually get into "if (EndsWithSequential)" and the Src (%arrayidx8) is merged with GEP (%tmp1 and %tmp2). Looks like there no such check we have in this patch in "if (EndsWithSequential)". 


https://reviews.llvm.org/D52671