[EarlyCSE] Fix dead store elimination for unwinding readnone calls

[nikic]

EarlyCSE already resets LastStore when it hits an potentially unwinding instruction, as the memory state may be observed by the caller after the unwind.

There also was a test specifically making sure that this works even for unwinding readnone calls -- however, the call in that test did not participate in EarlyCSE in the first place, because it returns void (relaxing that is how I got here), so it was not actually testing the right thing.

Move the check for unwinding instructions earlier, so it also handles the readnone case.

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

Author: Nikita Popov (nikic)

Changes

EarlyCSE already resets LastStore when it hits an potentially unwinding instruction, as the memory state may be observed by the caller after the unwind.

There also was a test specifically making sure that this works even for unwinding readnone calls -- however, the call in that test did not participate in EarlyCSE in the first place, because it returns void (relaxing that is how I got here), so it was not actually testing the right thing.

Move the check for unwinding instructions earlier, so it also handles the readnone case.

---

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

3 Files Affected:

• (modified) llvm/lib/Transforms/Scalar/EarlyCSE.cpp (+7-3)
• (modified) llvm/test/Transforms/EarlyCSE/basic.ll (+18-3)
• (removed) llvm/test/Transforms/EarlyCSE/readnone-mayunwind.ll (-18)

diff --git a/llvm/lib/Transforms/Scalar/EarlyCSE.cpp b/llvm/lib/Transforms/Scalar/EarlyCSE.cpp
index 5c62a2cf526e9..e1e283f171d38 100644
--- a/llvm/lib/Transforms/Scalar/EarlyCSE.cpp
+++ b/llvm/lib/Transforms/Scalar/EarlyCSE.cpp
@@ -1525,6 +1525,11 @@ bool EarlyCSE::processNode(DomTreeNode *Node) {
       }
     }
 
+    // Make sure stores prior to a potential unwind are not removed, as the
+    // caller may read the memory.
+    if (Inst.mayThrow())
+      LastStore = nullptr;
+
     // If this is a simple instruction that we can value number, process it.
     if (SimpleValue::canHandle(&Inst)) {
       if ([[maybe_unused]] auto *CI = dyn_cast<ConstrainedFPIntrinsic>(&Inst)) {
@@ -1616,13 +1621,12 @@ bool EarlyCSE::processNode(DomTreeNode *Node) {
       continue;
     }
 
-    // If this instruction may read from memory or throw (and potentially read
-    // from memory in the exception handler), forget LastStore.  Load/store
+    // If this instruction may read from memory, forget LastStore.  Load/store
     // intrinsics will indicate both a read and a write to memory.  The target
     // may override this (e.g. so that a store intrinsic does not read from
     // memory, and thus will be treated the same as a regular store for
     // commoning purposes).
-    if ((Inst.mayReadFromMemory() || Inst.mayThrow()) &&
+    if (Inst.mayReadFromMemory() &&
         !(MemInst.isValid() && !MemInst.mayReadFromMemory()))
       LastStore = nullptr;
 
diff --git a/llvm/test/Transforms/EarlyCSE/basic.ll b/llvm/test/Transforms/EarlyCSE/basic.ll
index c6b746026c94d..2c6b2a9613924 100644
--- a/llvm/test/Transforms/EarlyCSE/basic.ll
+++ b/llvm/test/Transforms/EarlyCSE/basic.ll
@@ -137,7 +137,7 @@ declare i32 @func(ptr%P) readonly
 ;; Simple call CSE'ing.
 define i32 @test5(ptr%P) {
 ; CHECK-LABEL: @test5(
-; CHECK-NEXT:    [[V1:%.*]] = call i32 @func(ptr [[P:%.*]]), !prof !0
+; CHECK-NEXT:    [[V1:%.*]] = call i32 @func(ptr [[P:%.*]]), !prof [[PROF0:![0-9]+]]
 ; CHECK-NEXT:    ret i32 0
 ;
   %V1 = call i32 @func(ptr %P), !prof !0
@@ -212,10 +212,25 @@ define i32 @test9(ptr%P) {
   ret i32 %V1
 }
 
-;; Trivial DSE can be performed across a readnone call.
+;; Trivial DSE can be performed across a readnone nounwind call.
 define i32 @test10(ptr%P) {
 ; CHECK-LABEL: @test10(
-; CHECK-NEXT:    [[V1:%.*]] = call i32 @func(ptr [[P:%.*]]) #[[ATTR2]]
+; CHECK-NEXT:    [[V1:%.*]] = call i32 @func(ptr [[P:%.*]]) #[[ATTR3:[0-9]+]]
+; CHECK-NEXT:    store i32 5, ptr [[P]], align 4
+; CHECK-NEXT:    ret i32 [[V1]]
+;
+  store i32 4, ptr %P
+  %V1 = call i32 @func(ptr %P) readnone nounwind
+  store i32 5, ptr %P
+  ret i32 %V1
+}
+
+; Trivial DSE can't be performed across a potentially unwinding readnone
+; call, as the caller may read the memory on unwind.
+define i32 @test_readnone_missing_nounwind(ptr %P) {
+; CHECK-LABEL: @test_readnone_missing_nounwind(
+; CHECK-NEXT:    store i32 4, ptr [[P:%.*]], align 4
+; CHECK-NEXT:    [[V1:%.*]] = call i32 @func(ptr [[P]]) #[[ATTR2]]
 ; CHECK-NEXT:    store i32 5, ptr [[P]], align 4
 ; CHECK-NEXT:    ret i32 [[V1]]
 ;
diff --git a/llvm/test/Transforms/EarlyCSE/readnone-mayunwind.ll b/llvm/test/Transforms/EarlyCSE/readnone-mayunwind.ll
deleted file mode 100644
index e4d31f31d9ff7..0000000000000
--- a/llvm/test/Transforms/EarlyCSE/readnone-mayunwind.ll
+++ /dev/null
@@ -1,18 +0,0 @@
-; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt -S -passes=early-cse -earlycse-debug-hash < %s | FileCheck %s
-
-declare void @readnone_may_unwind() readnone
-
-define void @f(ptr %ptr) {
-; CHECK-LABEL: @f(
-; CHECK-NEXT:    store i32 100, ptr [[PTR:%.*]], align 4
-; CHECK-NEXT:    call void @readnone_may_unwind()
-; CHECK-NEXT:    store i32 200, ptr [[PTR]], align 4
-; CHECK-NEXT:    ret void
-;
-
-  store i32 100, ptr %ptr
-  call void @readnone_may_unwind()
-  store i32 200, ptr %ptr
-  ret void
-}

[dtcxzyw]

All the target memory store intrinsics have nounwind, right?

[nikic]

I think so. It's the default for intrinsics.

[dtcxzyw]

LG

[fhahn]

LGTM, thanks