Skip to content

Conversation

@llvmbot
Copy link
Member

@llvmbot llvmbot commented Nov 21, 2024

Backport 18b02bb

Requested by: @dianqk

@llvmbot llvmbot added this to the LLVM 19.X Release milestone Nov 21, 2024
@llvmbot
Copy link
Member Author

llvmbot commented Nov 21, 2024

@nikic What do you think about merging this PR to the release branch?

@llvmbot llvmbot requested a review from nikic November 21, 2024 00:27
@llvmbot llvmbot added llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms labels Nov 21, 2024
@llvmbot
Copy link
Member Author

llvmbot commented Nov 21, 2024

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-llvm-analysis

Author: None (llvmbot)

Changes

Backport 18b02bb

Requested by: @DianQK


Full diff: https://github.com/llvm/llvm-project/pull/117082.diff

4 Files Affected:

  • (modified) llvm/include/llvm/Analysis/MemorySSAUpdater.h (+2-1)
  • (modified) llvm/lib/Analysis/MemorySSAUpdater.cpp (+5-3)
  • (modified) llvm/lib/Transforms/Scalar/LICM.cpp (+4-1)
  • (added) llvm/test/Transforms/LICM/PR116813-memoryssa-outdated.ll (+50)
diff --git a/llvm/include/llvm/Analysis/MemorySSAUpdater.h b/llvm/include/llvm/Analysis/MemorySSAUpdater.h
index d4da3ef1146db7..055feceefb0546 100644
--- a/llvm/include/llvm/Analysis/MemorySSAUpdater.h
+++ b/llvm/include/llvm/Analysis/MemorySSAUpdater.h
@@ -190,7 +190,8 @@ class MemorySSAUpdater {
   /// inaccessible and it *must* have removeMemoryAccess called on it.
   MemoryAccess *createMemoryAccessInBB(Instruction *I, MemoryAccess *Definition,
                                        const BasicBlock *BB,
-                                       MemorySSA::InsertionPlace Point);
+                                       MemorySSA::InsertionPlace Point,
+                                       bool CreationMustSucceed = true);
 
   /// Create a MemoryAccess in MemorySSA before an existing MemoryAccess.
   ///
diff --git a/llvm/lib/Analysis/MemorySSAUpdater.cpp b/llvm/lib/Analysis/MemorySSAUpdater.cpp
index aa550f0b6a7bfd..f672bd0e1e133e 100644
--- a/llvm/lib/Analysis/MemorySSAUpdater.cpp
+++ b/llvm/lib/Analysis/MemorySSAUpdater.cpp
@@ -1403,9 +1403,11 @@ void MemorySSAUpdater::changeToUnreachable(const Instruction *I) {
 
 MemoryAccess *MemorySSAUpdater::createMemoryAccessInBB(
     Instruction *I, MemoryAccess *Definition, const BasicBlock *BB,
-    MemorySSA::InsertionPlace Point) {
-  MemoryUseOrDef *NewAccess = MSSA->createDefinedAccess(I, Definition);
-  MSSA->insertIntoListsForBlock(NewAccess, BB, Point);
+    MemorySSA::InsertionPlace Point, bool CreationMustSucceed) {
+  MemoryUseOrDef *NewAccess = MSSA->createDefinedAccess(
+      I, Definition, /*Template=*/nullptr, CreationMustSucceed);
+  if (NewAccess)
+    MSSA->insertIntoListsForBlock(NewAccess, BB, Point);
   return NewAccess;
 }
 
diff --git a/llvm/lib/Transforms/Scalar/LICM.cpp b/llvm/lib/Transforms/Scalar/LICM.cpp
index 91ef2b4b7c1839..ca03eff7a4e25f 100644
--- a/llvm/lib/Transforms/Scalar/LICM.cpp
+++ b/llvm/lib/Transforms/Scalar/LICM.cpp
@@ -1464,8 +1464,11 @@ static Instruction *cloneInstructionInExitBlock(
 
   if (MSSAU.getMemorySSA()->getMemoryAccess(&I)) {
     // Create a new MemoryAccess and let MemorySSA set its defining access.
+    // After running some passes, MemorySSA might be outdated, and the
+    // instruction `I` may have become a non-memory touching instruction.
     MemoryAccess *NewMemAcc = MSSAU.createMemoryAccessInBB(
-        New, nullptr, New->getParent(), MemorySSA::Beginning);
+        New, nullptr, New->getParent(), MemorySSA::Beginning,
+        /*CreationMustSucceed=*/false);
     if (NewMemAcc) {
       if (auto *MemDef = dyn_cast<MemoryDef>(NewMemAcc))
         MSSAU.insertDef(MemDef, /*RenameUses=*/true);
diff --git a/llvm/test/Transforms/LICM/PR116813-memoryssa-outdated.ll b/llvm/test/Transforms/LICM/PR116813-memoryssa-outdated.ll
new file mode 100644
index 00000000000000..a040c3cc6947c6
--- /dev/null
+++ b/llvm/test/Transforms/LICM/PR116813-memoryssa-outdated.ll
@@ -0,0 +1,50 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -passes='loop-mssa(simple-loop-unswitch<nontrivial>,licm)' -verify-memoryssa -S < %s | FileCheck %s
+
+; Check that running LICM after SimpleLoopUnswitch does not result in a crash.
+
+define i32 @foo(i1 %arg, ptr %arg1) {
+; CHECK-LABEL: define i32 @foo(
+; CHECK-SAME: i1 [[ARG:%.*]], ptr [[ARG1:%.*]]) {
+; CHECK-NEXT:  [[START:.*:]]
+; CHECK-NEXT:    [[ARG_FR:%.*]] = freeze i1 [[ARG]]
+; CHECK-NEXT:    br i1 [[ARG_FR]], label %[[START_SPLIT_US:.*]], label %[[START_SPLIT:.*]]
+; CHECK:       [[START_SPLIT_US]]:
+; CHECK-NEXT:    br label %[[LOOP_US:.*]]
+; CHECK:       [[LOOP_US]]:
+; CHECK-NEXT:    br label %[[BB0:.*]]
+; CHECK:       [[BB0]]:
+; CHECK-NEXT:    br label %[[BB1:.*]]
+; CHECK:       [[BB1]]:
+; CHECK-NEXT:    [[UNSWITCHED_SELECT_US:%.*]] = phi ptr [ [[ARG1]], %[[BB0]] ]
+; CHECK-NEXT:    [[I3_US:%.*]] = call i32 [[UNSWITCHED_SELECT_US]]()
+; CHECK-NEXT:    br i1 true, label %[[LOOP_US]], label %[[RET_SPLIT_US:.*]]
+; CHECK:       [[RET_SPLIT_US]]:
+; CHECK-NEXT:    [[I3_LCSSA_US:%.*]] = phi i32 [ [[I3_US]], %[[BB1]] ]
+; CHECK-NEXT:    br label %[[RET:.*]]
+; CHECK:       [[START_SPLIT]]:
+; CHECK-NEXT:    br label %[[LOOP:.*]]
+; CHECK:       [[LOOP]]:
+; CHECK-NEXT:    br label %[[BB2:.*]]
+; CHECK:       [[BB2]]:
+; CHECK-NEXT:    br i1 false, label %[[LOOP]], label %[[RET_SPLIT:.*]]
+; CHECK:       [[RET_SPLIT]]:
+; CHECK-NEXT:    [[I3_LE:%.*]] = call i32 @bar()
+; CHECK-NEXT:    br label %[[RET]]
+; CHECK:       [[RET]]:
+; CHECK-NEXT:    [[DOTUS_PHI:%.*]] = phi i32 [ [[I3_LE]], %[[RET_SPLIT]] ], [ [[I3_LCSSA_US]], %[[RET_SPLIT_US]] ]
+; CHECK-NEXT:    ret i32 [[DOTUS_PHI]]
+;
+start:
+  br label %loop
+
+loop:                                              ; preds = %loop, %bb
+  %i = select i1 %arg, ptr %arg1, ptr @bar
+  %i3 = call i32 %i()
+  br i1 %arg, label %loop, label %ret
+
+ret:                                              ; preds = %loop
+  ret i32 %i3
+}
+
+declare i32 @bar() nounwind willreturn memory(none)

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Fixes llvm#116809.

After running some passes (SimpleLoopUnswitch, LoopInstSimplify, etc.),
MemorySSA might be outdated, and the instruction `I` may have become a
non-memory touching instruction.

LICM has already handled this, but it does not pass
`CreationMustSucceed=false` to `createDefinedAccess`.

(cherry picked from commit 18b02bb)
@tru tru merged commit 5bd0474 into llvm:release/19.x Nov 25, 2024
@github-actions
Copy link

@dianqk (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. When you are done, please add the release:note label to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

llvm:analysis Includes value tracking, cost tables and constant folding llvm:transforms

Projects

Development

Successfully merging this pull request may close these issues.

4 participants