[LICM] allow MemoryAccess creation failure #116813

DianQK · 2024-11-19T14:29:48Z

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.

…a BB

cloned BB

llvmbot · 2024-11-19T14:30:29Z

@llvm/pr-subscribers-llvm-analysis

@llvm/pr-subscribers-llvm-transforms

Author: DianQK (DianQK)

Changes

Fixes #116809.

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

2 Files Affected:

(modified) llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp (+2-2)
(added) llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll (+24)

diff --git a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
index aa3cbc5e4bddc2..d807c66bd96195 100644
--- a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
+++ b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
@@ -1678,7 +1678,7 @@ deleteDeadClonedBlocks(Loop &L, ArrayRef<BasicBlock *> ExitBlocks,
       if (BasicBlock *ClonedBB = cast_or_null<BasicBlock>(VMap->lookup(BB)))
         if (!DT.isReachableFromEntry(ClonedBB)) {
           for (BasicBlock *SuccBB : successors(ClonedBB))
-            SuccBB->removePredecessor(ClonedBB);
+            SuccBB->removePredecessor(ClonedBB, /*KeepOneInputPHIs*/ true);
           DeadBlocks.push_back(ClonedBB);
         }
 
@@ -1716,7 +1716,7 @@ static void deleteDeadBlocksFromLoop(Loop &L,
     auto *BB = DeathCandidates.pop_back_val();
     if (!DeadBlockSet.count(BB) && !DT.isReachableFromEntry(BB)) {
       for (BasicBlock *SuccBB : successors(BB)) {
-        SuccBB->removePredecessor(BB);
+        SuccBB->removePredecessor(BB, /*KeepOneInputPHIs*/ true);
         DeathCandidates.push_back(SuccBB);
       }
       DeadBlockSet.insert(BB);
diff --git a/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll b/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll
new file mode 100644
index 00000000000000..3545492f1e5e23
--- /dev/null
+++ b/llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll
@@ -0,0 +1,24 @@
+; RUN: opt -passes='loop-mssa(simple-loop-unswitch<nontrivial>),print<memoryssa>' -verify-memoryssa -disable-output -S < %s 2>&1 | FileCheck %s
+; RUN: opt -passes='loop-mssa(simple-loop-unswitch<nontrivial>,licm)' -verify-memoryssa -disable-output -S 
+
+; Check that SimpleLoopUnswitch preserves the MemoryDef of `call i32 @bar()` by preserving the PHI node.
+; Also, check that executing LICM after SimpleLoopUnswitch does not result in a crash.
+
+; CHECK: %unswitched.select = phi ptr [ @bar, %bb2 ]
+; CHECK-NEXT: MemoryDef
+; CHECK-NEXT: call i32 %unswitched.select()
+
+define i32 @foo(i1 %arg, ptr %arg1) {
+bb:
+  br label %bb2
+
+bb2:                                              ; preds = %bb2, %bb
+  %i = select i1 %arg, ptr %arg1, ptr @bar
+  %i3 = call i32 %i()
+  br i1 %arg, label %bb2, label %bb4
+
+bb4:                                              ; preds = %bb2
+  ret i32 %i3
+}
+
+declare i32 @bar() nounwind willreturn memory(none)

dtcxzyw · 2024-11-19T14:32:14Z

Does it fix #116228?

DianQK · 2024-11-19T14:30:57Z

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp

@@ -1678,7 +1678,7 @@ deleteDeadClonedBlocks(Loop &L, ArrayRef<BasicBlock *> ExitBlocks,
      if (BasicBlock *ClonedBB = cast_or_null<BasicBlock>(VMap->lookup(BB)))
        if (!DT.isReachableFromEntry(ClonedBB)) {
          for (BasicBlock *SuccBB : successors(ClonedBB))
-            SuccBB->removePredecessor(ClonedBB);


I don't have a test case for now yet, but I think we also need to keep one phi here.

DianQK · 2024-11-19T14:31:56Z

llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll

+; Also, check that executing LICM after SimpleLoopUnswitch does not result in a crash.
+
+; CHECK: %unswitched.select = phi ptr [ @bar, %bb2 ]
+; CHECK-NEXT: MemoryDef


I cannot use /llvm/utils/update_test_checks.py because I want to check MemoryDef here.

DianQK · 2024-11-19T14:33:28Z

llvm/test/Transforms/SimpleLoopUnswitch/nontrivial-unswitch-pred-removed.ll

@@ -0,0 +1,24 @@
+; RUN: opt -passes='loop-mssa(simple-loop-unswitch<nontrivial>),print<memoryssa>' -verify-memoryssa -disable-output -S < %s 2>&1 | FileCheck %s
+; RUN: opt -passes='loop-mssa(simple-loop-unswitch<nontrivial>,licm)' -verify-memoryssa -disable-output -S 


If possible, I like /llvm/utils/update_test_checks.py, but verifying that it does not crash is sufficient.

DianQK · 2024-11-19T14:35:21Z

llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp

@@ -1716,7 +1716,7 @@ static void deleteDeadBlocksFromLoop(Loop &L,
    auto *BB = DeathCandidates.pop_back_val();
    if (!DeadBlockSet.count(BB) && !DT.isReachableFromEntry(BB)) {
      for (BasicBlock *SuccBB : successors(BB)) {
-        SuccBB->removePredecessor(BB);
+        SuccBB->removePredecessor(BB, /*KeepOneInputPHIs*/ true);
        DeathCandidates.push_back(SuccBB);


Question: why can't MemorySSA be updated here? Is it because MemorySSAUpdater does not yet support this?

DianQK · 2024-11-19T14:41:38Z

Does it fix #116228?

~~Maybe. It also hit other assertions. :(~~

Edit: I think this is a different issue.

nikic

Can you add more detail to the PR description to explain why you think this is the right fix? This is not how such assertion failures are usually addressed.

nikic

If I understand correctly, you're basically trying to fix this problem by preventing simplifications that may make a call memory(none) from occurring. This is definitely not a battle you can win :) SimpleLoopUnswitch is not the only pass that can do such simplifications. LoopInstSimplify is usually the easier way to produce such cases.

The more robust way to fix this is, when creating a new memory access that is essentially a clone of an existing one, to pass CreationMustSucceed=false to createDefinedAccess() and then handle no access being created.

The relevant code here (

llvm-project/llvm/lib/Transforms/Scalar/LICM.cpp

Line 1468 in 0611a66

MemoryAccess *NewMemAcc = MSSAU.createMemoryAccessInBB(

) already handles no access being created, but the API it goes through (createMemoryAccessInBB) currently doesn't have the CreationMustSucceed flag, so you'll have to add it first.

…ssor of a" This reverts commit 6b80bdb.

…ssor of a BB" This reverts commit b164143.

DianQK · 2024-11-19T22:52:34Z

If I understand correctly, you're basically trying to fix this problem by preventing simplifications that may make a call memory(none) from occurring. This is definitely not a battle you can win :) SimpleLoopUnswitch is not the only pass that can do such simplifications. LoopInstSimplify is usually the easier way to produce such cases.

The more robust way to fix this is, when creating a new memory access that is essentially a clone of an existing one, to pass CreationMustSucceed=false to createDefinedAccess() and then handle no access being created.

The relevant code here (

llvm-project/llvm/lib/Transforms/Scalar/LICM.cpp

Line 1468 in 0611a66

MemoryAccess *NewMemAcc = MSSAU.createMemoryAccessInBB(

) already handles no access being created, but the API it goes through (createMemoryAccessInBB) currently doesn't have the CreationMustSucceed flag, so you'll have to add it first.

That was my initial thought. But, as I kept digging into the root cause of the failure, I noticed the MemorySSA being outdated. Then, I found that many calls to removePredecessor(BB, /*KeepOneInputPHIs*/ true) so I made this somewhat wired PR. Upon reconsideration, I realized I could think this outdated result as a conservative analysis result.

nikic

LGTM

llvm/test/Transforms/LICM/PR116813-memoryssa-outdated.ll

DianQK · 2024-11-21T00:22:13Z

/cherry-pick 18b02bb

llvmbot · 2024-11-21T00:27:38Z

/pull-request #117082

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)

DianQK added 3 commits November 19, 2024 22:01

Pre-commit test cases

b2eb1f4

[SimpleLoopUnswitch] Preserve one PHI when removing a predecessor of …

b164143

…a BB

[SimpleLoopUnswitch] Preserve one PHI when removing a predecessor of a

6b80bdb

cloned BB

DianQK requested review from nikic, fhahn, dtcxzyw and AmrDeveloper November 19, 2024 14:29

llvmbot added the llvm:transforms label Nov 19, 2024

DianQK changed the title ~~[SimpleLoopUnswitch] Preserve one PHI when removing a predecessor of a BB~~ [SimpleLoopUnswitch] Keep one PHI when removing a predecessor of a BB Nov 19, 2024

DianQK commented Nov 19, 2024

View reviewed changes

nikic reviewed Nov 19, 2024

View reviewed changes

nikic requested changes Nov 19, 2024

View reviewed changes

DianQK added 3 commits November 20, 2024 05:43

Revert "[SimpleLoopUnswitch] Preserve one PHI when removing a predece…

5d2867b

…ssor of a" This reverts commit 6b80bdb.

Revert "[SimpleLoopUnswitch] Preserve one PHI when removing a predece…

e6b291a

…ssor of a BB" This reverts commit b164143.

[LICM] allow MemoryAccess creation failure

e553a41

llvmbot added the llvm:analysis label Nov 19, 2024

DianQK changed the title ~~[SimpleLoopUnswitch] Keep one PHI when removing a predecessor of a BB~~ [LICM] allow MemoryAccess creation failure Nov 19, 2024

nikic approved these changes Nov 20, 2024

View reviewed changes

llvm/test/Transforms/LICM/PR116813-memoryssa-outdated.ll Outdated Show resolved Hide resolved

Update test

de04782

DianQK merged commit 18b02bb into llvm:main Nov 20, 2024
5 of 8 checks passed

DianQK deleted the simple-loop-unswitch-memory-ssa branch November 20, 2024 11:52

DianQK added this to the LLVM 19.X Release milestone Nov 21, 2024

thewtex mentioned this pull request Feb 10, 2025

llvmorg 19.1.5 libcxxabi pthread lib name #126605

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LICM] allow MemoryAccess creation failure #116813

[LICM] allow MemoryAccess creation failure #116813

DianQK commented Nov 19, 2024 •

edited

Loading

llvmbot commented Nov 19, 2024 •

edited

Loading

dtcxzyw commented Nov 19, 2024

DianQK Nov 19, 2024

DianQK Nov 19, 2024

DianQK Nov 19, 2024

DianQK Nov 19, 2024

DianQK commented Nov 19, 2024 •

edited

Loading

nikic left a comment

nikic left a comment

DianQK commented Nov 19, 2024

nikic left a comment

DianQK commented Nov 21, 2024

llvmbot commented Nov 21, 2024

		@@ -0,0 +1,24 @@
		; RUN: opt -passes='loop-mssa(simple-loop-unswitch<nontrivial>),print<memoryssa>' -verify-memoryssa -disable-output -S < %s 2>&1 \| FileCheck %s
		; RUN: opt -passes='loop-mssa(simple-loop-unswitch<nontrivial>,licm)' -verify-memoryssa -disable-output -S

[LICM] allow MemoryAccess creation failure #116813

[LICM] allow MemoryAccess creation failure #116813

Conversation

DianQK commented Nov 19, 2024 • edited Loading

llvmbot commented Nov 19, 2024 • edited Loading

dtcxzyw commented Nov 19, 2024

DianQK Nov 19, 2024

Choose a reason for hiding this comment

DianQK Nov 19, 2024

Choose a reason for hiding this comment

DianQK Nov 19, 2024

Choose a reason for hiding this comment

DianQK Nov 19, 2024

Choose a reason for hiding this comment

DianQK commented Nov 19, 2024 • edited Loading

nikic left a comment

Choose a reason for hiding this comment

nikic left a comment

Choose a reason for hiding this comment

DianQK commented Nov 19, 2024

nikic left a comment

Choose a reason for hiding this comment

DianQK commented Nov 21, 2024

llvmbot commented Nov 21, 2024

DianQK commented Nov 19, 2024 •

edited

Loading

llvmbot commented Nov 19, 2024 •

edited

Loading

DianQK commented Nov 19, 2024 •

edited

Loading