[LIR] Fix mis-compilation with unwinding

[haicheng]

Summary

To fix PR27859, bail out if there is an instruction may throw.

[mcrosier]

Looks conservatively correct and GTM. What do you think @eli.friedman?

[majnemer]

I'm pretty sure this is wrong. It only considers calls in the same basic block as the store. However, the store could post-dominate a call to a function which never returns.

Instead, I suggest using llvm::isGuaranteedToTransferExecutionToSuccessor instead of the current, wrong, heuristic of "does this BB dominate all the exit blocks".

Due to the nature of llvm::isGuaranteedToTransferExecutionToSuccessor, I suggest that it's logic be factored out so that you can cache some of the intermediate results.

test/Transforms/LoopIdiom/unwind.ll

5–51

There is no need to provide the definition of f. You could use my reduced testcase, I'm pretty sure it is minimal.

[haicheng]

test/Transforms/LoopIdiom/unwind.ll

5–51

Targets like AArch64 do not generate attribute uwtable so that I include the definition of f.

[majnemer]

test/Transforms/LoopIdiom/unwind.ll

5–51

Er, this testcase has an explicit triple. What does AArch64 have to do with anything?

[eli.friedman]

lib/Transforms/Scalar/LoopIdiomRecognize.cpp

392

This check is in collectStores, which is run once per basic block... so this won't notice a call which throws in a different basic block ("for (...) { p[i] = 0; if (a) throwing_fn(); }"),

test/Transforms/LoopIdiom/unwind.ll

19

No reason to include the implementation of f in the testcase.

[eli.friedman]

Are you planning to continue working on this?

[haicheng]

Yes, but not in a week or two. I am tied up with some other projects.

[mssimpso]

Resigning since David and Eli are reviewing.

[haicheng]

Sorry for the long delay.

Just use computeLoopSafetyInfo (which calls isGuaranteedToTransferExecutionToSuccessor) to decide whether the loop may throw or not.

Please take a look.

[eli.friedman]

The code looks fine. The comments could use a bit of refinement.

lib/Transforms/Scalar/LoopIdiomRecognize.cpp

220

Useless comment.

223

It would be useful to explain why this check is necessary. Among other things, it would be good to mention that existing transforms involve hoisting stores into the loop pre-header.

[haicheng]

Refine the comments as suggested by Eli.

[eli.friedman]

LGTM.

lib/Transforms/Scalar/LoopIdiomRecognize.cpp

bool LoopIdiomRecognize::runOnCountableLoop() {
   SmallVector<BasicBlock *, 8> ExitBlocks;
   CurLoop->getUniqueExitBlocks(ExitBlocks);
 
   DEBUG(dbgs() << "loop-idiom Scanning: F["
                << CurLoop->getHeader()->getParent()->getName() << "] Loop %"
                << CurLoop->getHeader()->getName() << "\n");
 
   bool MadeChange = false;
+
+  // Compute loop safety information.
+  LoopSafetyInfo SafetyInfo;
+  computeLoopSafetyInfo(&SafetyInfo, CurLoop);
+  // Give up if the loop may throw.
+  if (SafetyInfo.MayThrow)
+    return MadeChange;
+
   // Scan all the blocks in the loop that are not in subloops.
   for (auto *BB : CurLoop->getBlocks()) {
     // Ignore blocks in subloops.
     if (LI->getLoopFor(BB) != CurLoop)
       continue;
 
     MadeChange |= runOnLoopBlock(BB, BECount, ExitBlocks);
   }
void LoopIdiomRecognize::collectStores(BasicBlock *BB) {
   StoreRefsForMemsetPattern.clear();
   StoreRefsForMemcpy.clear();
   for (Instruction &I : *BB) {
     StoreInst *SI = dyn_cast<StoreInst>(&I);
     if (!SI)
       continue;
 
     bool ForMemset = false;
     bool ForMemsetPattern = false;
     bool ForMemcpy = false;
     // Make sure this is a strided store with a constant stride.
     if (!isLegalStore(SI, ForMemset, ForMemsetPattern, ForMemcpy))
       continue;
 
     // Save the store locations.
     if (ForMemset) {
       // Find the base pointer.

test/Transforms/LoopIdiom/unwind.ll

+; RUN: opt -loop-idiom < %s -S | FileCheck %s
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare void @ff()
+
+define void @test(i8* noalias nocapture %base, i64 %size) #1 {
+entry:
+  %cmp3 = icmp eq i64 %size, 0
+  br i1 %cmp3, label %for.end, label %for.body.preheader
+
+for.body.preheader:                               ; preds = %entry
+  br label %for.body
+
+for.body:                                         ; preds = %for.body.preheader, %for.body
+; CHECK-LABEL: @test(
+; CHECK-NOT: llvm.memset
+  %indvars.iv = phi i64 [ 0, %for.body.preheader ], [ %indvars.iv.next, %for.body ]
+  tail call void @ff()
+  %arrayidx = getelementptr inbounds i8, i8* %base, i64 %indvars.iv
+  store i8 0, i8* %arrayidx, align 1
+  %indvars.iv.next = add i64 %indvars.iv, 1
+  %exitcond = icmp ne i64 %indvars.iv.next, %size
+  br i1 %exitcond, label %for.body, label %for.end.loopexit
+
+for.end.loopexit:                                 ; preds = %for.body
+  br label %for.end
+
+for.end:                                          ; preds = %for.end.loopexit, %entry
+  ret void
+}
+
+attributes #1 = { uwtable }