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

[Eli Friedman via Phabricator via llvm-commits]

efriedma 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();
----------------
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.


https://reviews.llvm.org/D52671