[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:
> > > 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)". 
> Oh, sorry, you're right, I managed to confuse myself based on what we should be doing, rather than what we're actually doing at the moment.
> 
> I don't like adding heuristics based on the uses of the GEP; that's likely to lead to inconsistent results, and possibly O(N^2) compile-time.
> 
> What I think we should be doing instead is trying to maintain the invariant that each GEP either has all constant offsets, or has exactly one non-zero offset. Roughly, the rule allows GEPs which will lower to one addition instruction.  It composes well, other optimizations can already deal with it (IIRC), and it's a straightforward extension of what we're already doing in the "if (EndsWithSequential)" block.
>What I think we should be doing instead is trying to maintain the invariant that each GEP either has all constant offsets, or has exactly one non-zero offset. 

I'm afraid if I fully catch what you meant here. I guess you meant that overall we need to be less aggressive in merging GEPs in instcombine, and in instcombine, we try to keep GEPs have either all constant offset or exactly one non-constant offset? If merging GEPs can break this, we do not merge. 

> It composes well, other optimizations can already deal with it (IIRC)
I guess CodeGenprepare and ISel.  Please let me know other passes which handle something like this. 



https://reviews.llvm.org/D52671