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

[Jun Bum Lim via Phabricator via llvm-commits]

junbuml added a comment.

> Do you have perf/size numbers?

I didn't see any degradation in size/perf in my spec2000. Just observed minor performance improvement in some internal benchmarks.

> I'm sort of confused that MachineCSE doesn't trigger on your testcase, but that isn't really relevant here, I guess.

I also think MachineCSE should be able to catch it in the machine level, but somehow it's not. I will check this too. However, I believe it's better not to distribute the same calculations in early phase unless we have clear benefits with the duplicated calculations.



================
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:
> 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? 


https://reviews.llvm.org/D52671