[llvm] a8ca404 - [LSR] Fix crash in Phi node with EHPad block

[Quentin Colombet via llvm-commits]

Author: Quentin Colombet
Date: 2022-01-14T18:53:18-08:00
New Revision: a8ca4046e22d79c0521df3e78924255362d2be4a

URL: https://github.com/llvm/llvm-project/commit/a8ca4046e22d79c0521df3e78924255362d2be4a
DIFF: https://github.com/llvm/llvm-project/commit/a8ca4046e22d79c0521df3e78924255362d2be4a.diff

LOG: [LSR] Fix crash in Phi node with EHPad block

This fixes a crash I observed in issue #48708 where the LSR
pass tries to insert an instruction in a basic block with only a
catchswitch statement in there. This happens because the Phi node
being evaluated assumes the same value for different basic blocks.

If the basic block associated with the incoming value of the operand
being evaluated has an EHPad terminator LSR skips optimizing it.
But if that incoming value can come from multiple different blocks
there can be some incoming basic blocks which are terminated in
an EHPad. If these are then rewritten in RewriteForPhi the ones
containing an EHPad terminator will hit the "Insertion point must
be a normal instruction" assert in AdjustInsertPositionForExpand.

This fix makes CollectLoopInvariantFixupsAndFormulae also ignore
cases where the same value has another incoming basic block with an
EHPad, same as it already does in case the primary value has one.

Patch by Lorenz Brun <lorenz at brun.one>

Differential Revision: https://reviews.llvm.org/D98378

Added: 
    llvm/test/Transforms/LoopStrengthReduce/phi_ehpad_ignore_sameval.ll

Modified: 
    llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp b/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
index 798af48c23379..654f0d2a03a89 100644
--- a/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopStrengthReduce.cpp
@@ -3486,6 +3486,31 @@ LSRInstance::CollectLoopInvariantFixupsAndFormulae() {
         // Don't bother if the instruction is in a BB which ends in an EHPad.
         if (UseBB->getTerminator()->isEHPad())
           continue;
+
+        // Ignore cases in which the currently-examined value could come from
+        // a basic block terminated with an EHPad. This checks all incoming
+        // blocks of the phi node since it is possible that the same incoming
+        // value comes from multiple basic blocks, only some of which may end
+        // in an EHPad. If any of them do, a subsequent rewrite attempt by this
+        // pass would try to insert instructions into an EHPad, hitting an
+        // assertion.
+        if (isa<PHINode>(UserInst)) {
+          const auto *PhiNode = cast<PHINode>(UserInst);
+          bool HasIncompatibleEHPTerminatedBlock = false;
+          llvm::Value *ExpectedValue = U;
+          for (unsigned int I = 0; I < PhiNode->getNumIncomingValues(); I++) {
+            if (PhiNode->getIncomingValue(I) == ExpectedValue) {
+              if (PhiNode->getIncomingBlock(I)->getTerminator()->isEHPad()) {
+                HasIncompatibleEHPTerminatedBlock = true;
+                break;
+              }
+            }
+          }
+          if (HasIncompatibleEHPTerminatedBlock) {
+            continue;
+          }
+        }
+
         // Don't bother rewriting PHIs in catchswitch blocks.
         if (isa<CatchSwitchInst>(UserInst->getParent()->getTerminator()))
           continue;

diff  --git a/llvm/test/Transforms/LoopStrengthReduce/phi_ehpad_ignore_sameval.ll b/llvm/test/Transforms/LoopStrengthReduce/phi_ehpad_ignore_sameval.ll
new file mode 100644
index 0000000000000..4775f03357fa6
--- /dev/null
+++ b/llvm/test/Transforms/LoopStrengthReduce/phi_ehpad_ignore_sameval.ll
@@ -0,0 +1,55 @@
+; This tests that LoopStrengthReduce ignores possible optimizations that are
+; not realizable because they would require rewriting EHPad-class instructions.
+; If this type of optimization is attempted it will hit the
+; "Insertion point must be a normal instruction" assertion.
+;
+; See also https://bugs.llvm.org/show_bug.cgi?id=48708
+;
+; RUN: opt -loop-reduce -S %s
+
+target datalayout = "e-m:w-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:6:2:64-S128"
+
+%"class.std::allocator" = type { i8 }
+%"class.absl::Storage" = type {}
+
+define void @0() personality i8* undef {
+init1:
+  %i14 = invoke i8* undef(i8* null, i8 0)
+          to label %init2 unwind label %unwind
+
+init2:                                            ; preds = %init1
+  %i19 = select i1 undef, %"class.std::allocator"* null, %"class.std::allocator"* null
+  br label %loop
+
+loop:                                             ; preds = %loop.increment, %init2
+  %i21 = phi i64 [ %i24, %loop.increment ], [ 0, %init2 ]
+  %i22 = getelementptr %"class.std::allocator", %"class.std::allocator"* %i19, i64 %i21
+  invoke void undef(i8* null, %"class.std::allocator"* null, %"class.std::allocator"* %i22)
+          to label %loop.increment unwind label %loop.unwind
+
+loop.increment:                                   ; preds = %loop
+  %i24 = add i64 %i21, 1
+  br label %loop
+
+loop.unwind:                                      ; preds = %loop
+  %i26 = catchswitch within none [label %loop.catch] unwind label %unwind
+
+loop.catch:                                       ; preds = %loop.unwind
+  %i28 = catchpad within %i26 []
+  catchret from %i28 to label %caught
+
+caught:                                           ; preds = %loop.catch
+  invoke void undef(%"class.absl::Storage"* null)
+          to label %unreach unwind label %unwind
+
+unreach:                                          ; preds = %caught
+  unreachable
+
+unwind:                                           ; preds = %caught, %loop.unwind, %init1
+  ; This phi node triggers the issue in combination with the optimizable loop
+  ; above. It contains %i19 twice, once from %caught (which doesn't have an
+  ; EHPad) and once from %loop.unwind, which does have one.
+  %i32 = phi %"class.std::allocator"* [ %i19, %loop.unwind ], [ %i19, %caught ], [ null, %init1 ]
+  %i33 = cleanuppad within none []
+  cleanupret from %i33 unwind to caller
+}