[LSR] Check if terminating value is safe to expand before transformation

[eopXD]

Summary

According to report by @JojoR, the assertion error was hit hence we need
to have this check before the actual transformation.

[fhahn]

According to report by @JojoR, the assertion error was hit hence we need to have this check before the actual transformation.

Would it be possible to add the test case?

[eopXD]

@JojoR May you please upload the LLVM IR with -O0 (so nothing is triggered) of what causes the assertion error to hit to here so I can try reduce the a test case out of it?

[eopXD]

Add test case reduced from FFmpeg/libavfilter/ebur128.c

[eopXD]

Gentle ping, thank you.

[JojoR]

In D136415#3877015, @eopXD wrote:

@JojoR May you please upload the LLVM IR with -O0 (so nothing is triggered) of what causes the assertion error to hit to here so I can try reduce the a test case out of it?

I tested again and the error is not triggered with -O0,
I reduce the test case, see attached, hope that it's helpful for you :)

reduced-alac.ll810 BDownload

[JojoR]

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6821

The patch looks good to me,
but the implementation is redundant with here ?
it should merge into one ?

[eopXD]

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6821

Sorry, may you explain more on the redundancy you see here.

[eopXD]

Add test case provided by @JojoR

[JojoR]

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6821

Looks only one expression, ignore this comment.
my original consideration is that put same action/checker into one place, it's ok also here :)

[Meinersbur]

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6821

Could you explain why the TermValueS that that is checked by Expander.isSafeToExpand before this patch is different than the TermValueS in IsToHelpFold after this patch?

llvm/test/Transforms/LoopStrengthReduce/lsr-term-fold-negative-testcase.ll

224 ↗

(On Diff #471034)

[nit]

[eopXD]

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6821

TermValueS calculation does not change before/after this patch. The addition here is to guard the expansion of it by checking if it is safe in the legalization of this terminating condition fold transformation.

[eopXD]

Address comments from Michael.

[syzaara]

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

6821

It seems like we are doing this SCEV expansion already in IsToHelpFold. Could we just return TermValueS from there rather than redoing the SCEV calculation again?

[eopXD]

Change: Address comment, avoid duplicate calculation for TermValueS.

[eopXD]

Ping for reviewal.

[Meinersbur]

LGTM

[eopXD]

Rebase to latest main.

llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

for (User *U : V->users()) {
       }
     }
     return VToPN && VToTermCond;
   };
 
   // For `IsToHelpFold`, other IV that is an affine AddRec will be sufficient to
   // replace the terminating condition
   auto IsToHelpFold = [&](PHINode &PN) -> bool {
+    const SCEVAddRecExpr *AddRec = cast<SCEVAddRecExpr>(SE.getSCEV(&PN));
+    const SCEV *BECount = SE.getBackedgeTakenCount(L);
+    const SCEV *TermValueS = SE.getAddExpr(
+        AddRec->getOperand(0),
+        SE.getTruncateOrZeroExtend(
+            SE.getMulExpr(
+                AddRec->getOperand(1),
+                SE.getTruncateOrZeroExtend(
+                    SE.getAddExpr(BECount, SE.getOne(BECount->getType())),
+                    AddRec->getOperand(1)->getType())),
+            AddRec->getOperand(0)->getType()));
+    const DataLayout &DL = L->getHeader()->getModule()->getDataLayout();
+    SCEVExpander Expander(SE, DL, "lsr_fold_term_cond");
+
     // TODO: Right now we limit the phi node to help the folding be of a start
     // value of getelementptr. We can extend to any kinds of IV as long as it is
     // an affine AddRec. Add a switch to cover more types of instructions here
     // and down in the actual transformation.
-    return isa<GetElementPtrInst>(PN.getIncomingValueForBlock(LoopPreheader));
+    return Expander.isSafeToExpand(TermValueS) &&
+           isa<GetElementPtrInst>(PN.getIncomingValueForBlock(LoopPreheader));
   };
 
   PHINode *ToFold = nullptr;
   PHINode *ToHelpFold = nullptr;
 
   for (PHINode &PN : L->getHeader()->phis()) {
     if (!SE.isSCEVable(PN.getType())) {
       LLVM_DEBUG(dbgs() << "IV of phi '" << PN
if (CanFoldTerminatingCondition) {
       GetElementPtrInst *StartValueGEP = cast<GetElementPtrInst>(StartValue);
       Type *PtrTy = StartValueGEP->getPointerOperand()->getType();
 
       const SCEV *BECount = SE.getBackedgeTakenCount(L);
       const SCEVAddRecExpr *AddRec =
           cast<SCEVAddRecExpr>(SE.getSCEV(ToHelpFold));
 
       // TermValue = Start + Stride * (BackedgeCount + 1)
       const SCEV *TermValueS = SE.getAddExpr(
           AddRec->getOperand(0),
           SE.getTruncateOrZeroExtend(
               SE.getMulExpr(
                   AddRec->getOperand(1),
                   SE.getTruncateOrZeroExtend(
                       SE.getAddExpr(BECount, SE.getOne(BECount->getType())),
                       AddRec->getOperand(1)->getType())),
               AddRec->getOperand(0)->getType()));
 
-      // NOTE: If this is triggered, we should add this into predicate
-      if (!Expander.isSafeToExpand(TermValueS)) {
-        LLVMContext &Ctx = L->getHeader()->getContext();
-        Ctx.emitError(
-            "Terminating value is not safe to expand, need to add it to "
-            "predicate");
-      } else { // Now we replace the condition with ToHelpFold and remove ToFold
-        Changed = true;
-        NumTermFold++;
 
-        Value *TermValue = Expander.expandCodeFor(
-            TermValueS, PtrTy, LoopPreheader->getTerminator());
+      Value *TermValue = Expander.expandCodeFor(TermValueS, PtrTy,
+                                                LoopPreheader->getTerminator());
 
-        LLVM_DEBUG(dbgs() << "Start value of new term-cond phi-node:\n"
-                          << *StartValue << "\n"
-                          << "Terminating value of new term-cond phi-node:\n"
-                          << *TermValue << "\n");
 
-        // Create new terminating condition at loop latch
-        BranchInst *BI = cast<BranchInst>(LoopLatch->getTerminator());
-        ICmpInst *OldTermCond = cast<ICmpInst>(BI->getCondition());
-        IRBuilder<> LatchBuilder(LoopLatch->getTerminator());
-        Value *NewTermCond = LatchBuilder.CreateICmp(
-            OldTermCond->getPredicate(), LoopValue, TermValue,
-            "lsr_fold_term_cond.replaced_term_cond");
 
-        LLVM_DEBUG(dbgs() << "Old term-cond:\n"
-                          << *OldTermCond << "\n"
-                          << "New term-cond:\b" << *NewTermCond << "\n");
 
-        BI->setCondition(NewTermCond);
 
-        OldTermCond->eraseFromParent();
-        DeleteDeadPHIs(L->getHeader(), &TLI, MSSAU.get());
-      }
 
       ExpCleaner.markResultUsed();
     }
   }
 
   if (SalvageableDVIRecords.empty())
     return Changed;