[InstCombine] Avoid folding GEPs across loop boundaries

[clin1]

Summary

Folding a GEP from outside to inside a loop will materialize an add where there wasn't an equivalent operation before. Check the containing loops before making this fold.

[clin1]

Forgot the check lines...

[clin1]

Failing "task-dependency.c" test looks like unrelated flakiness. Observed it pass/failing randomly in local testing.

[RKSimon]

Adding some likely reviewers (I'm currently away with only limited web access).

[lebedev.ri]

Hm, i thought i commented here, but alas not. LGTM.

I suspect while we want to merge this, it's only half of the story - what if LICM didn't run yet?
We need some kind of an undo transform. (and no, SeparateConstOffsetFromGEP doesn't help, i tried)

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

2135

[clin1]

Thank you for the review. Could I please ask someone to commit, with --author="Chang-Sun Lin, Jr. <[e-mail]>"?

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

if (Src->getNumOperands() == 2 && GEP.getNumOperands() == 2 &&
                   GetElementPtrInst::Create(GEPEltType, NewSrc, {SO1});
               NewGEP->setIsInBounds(GEP.isInBounds());
               return NewGEP;
             }
           }
         }
       }
 
+      // Guard the gep(gep) fold so we don't create an add inside a loop
+      // when there wasn't an equivalent instruction there before.
+      bool DifferentLoops = false;
+      if (LI)
+        if (auto *GEPLoop = LI->getLoopFor(GEP.getParent()))
+          if (auto *SrcOpI = dyn_cast<Instruction>(Src))
+            if (LI->getLoopFor(SrcOpI->getParent()) != GEPLoop)
+              DifferentLoops = true;
+
       // Fold (gep(gep(Ptr,Idx0),Idx1) -> gep(Ptr,add(Idx0,Idx1))
-      if (GO1->getType() == SO1->getType()) {
+      if (!DifferentLoops && GO1->getType() == SO1->getType()) {
         bool NewInBounds = GEP.isInBounds() && Src->isInBounds();
         auto *NewIdx =
             Builder.CreateAdd(GO1, SO1, GEP.getName() + ".idx",
                               /*HasNUW*/ false, /*HasNSW*/ NewInBounds);
         auto *NewGEP = GetElementPtrInst::Create(
             GEPEltType, Src->getPointerOperand(), {NewIdx});
         NewGEP->setIsInBounds(NewInBounds);
         return NewGEP;

llvm/test/Transforms/InstCombine/gep-combine-loop-invariant.ll

loop:
   %pi1 = ptrtoint <2 x i8**> %e4 to <2 x i64>
   %lr1 = lshr <2 x i64> %pi1, <i64 21, i64 21>
   %sl1 = shl nuw nsw <2 x i64> %lr1, <i64 7, i64 7>
   %e5 = getelementptr inbounds i8, <2 x i8*> %base, <2 x i64> %sl1
   %e6 = getelementptr inbounds i8, <2 x i8*> %e5, i64 80
   call void @blackhole(<2 x i8*> %e6)
   br label %loop
 }
+
+; Avoid folding the GEP outside the loop to inside, and increasing loop
+; instruction count.
+define float @gep_cross_loop(i64* %_arg_, float* %_arg_3, float %_arg_8)
+{
+entry:
+  %0 = load i64, i64* %_arg_, align 8
+  %add.ptr = getelementptr inbounds float, float* %_arg_3, i64 %0
+  br label %for.cond.i
+
+for.cond.i:                                       ; preds = %for.body.i, %entry
+  %idx = phi i64 [ 0, %entry ], [ %add11.i, %for.body.i ]
+  %sum = phi float [ 0.000000e+00, %entry ], [ %add.i, %for.body.i ]
+  %cmp = icmp ule i64 %idx, 16
+  br i1 %cmp, label %for.body.i, label %for.cond.i.i.i.preheader
+
+for.cond.i.i.i.preheader:                         ; preds = %for.cond.i
+  ret float %sum
+
+for.body.i:                                       ; preds = %for.cond.i
+  %arrayidx.i84.i = getelementptr inbounds float, float * %add.ptr, i64 %idx
+  %1 = load float, float* %arrayidx.i84.i, align 4
+  %add.i = fadd fast float %sum, %1
+  %add11.i = add nsw i64 %idx, 1
+  br label %for.cond.i
+}