[PATCH] D105343: [SimplifyCFG] Volatile memory operations do not trap

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri created this revision.
lebedev.ri added reviewers: spatel, nikic, efriedma, nlopes, stephan.yichao.zhao, chandlerc.
lebedev.ri added a project: LLVM.
Herald added subscribers: jfb, hiraditya.
lebedev.ri requested review of this revision.

Somewhat related to D105338 <https://reviews.llvm.org/D105338>.
While it is up for disscussion whether or not volatile store traps,
so far there has been no complaints that volatile load/cmpxchg/atomicrmw also may trap.
And even if simplifycfg currently concervatively believes that to be the case,
instcombine does not: https://godbolt.org/z/5vhv4K5b8


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105343

Files:
  llvm/lib/Transforms/Utils/SimplifyCFG.cpp
  llvm/test/Transforms/SimplifyCFG/trapping-load-unreachable.ll


Index: llvm/test/Transforms/SimplifyCFG/trapping-load-unreachable.ll
===================================================================
--- llvm/test/Transforms/SimplifyCFG/trapping-load-unreachable.ll
+++ llvm/test/Transforms/SimplifyCFG/trapping-load-unreachable.ll
@@ -10,11 +10,8 @@
 ; CHECK-LABEL: @test1(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[TMP0:%.*]] = icmp eq i32 [[X:%.*]], 0
-; CHECK-NEXT:    br i1 [[TMP0]], label [[BB:%.*]], label [[RETURN:%.*]]
-; CHECK:       bb:
-; CHECK-NEXT:    [[TMP1:%.*]] = load volatile i32, i32* null, align 4
-; CHECK-NEXT:    unreachable
-; CHECK:       return:
+; CHECK-NEXT:    [[TMP1:%.*]] = xor i1 [[TMP0]], true
+; CHECK-NEXT:    call void @llvm.assume(i1 [[TMP1]])
 ; CHECK-NEXT:    ret void
 ;
 entry:
@@ -34,11 +31,8 @@
 ; CHECK-LABEL: @test1_no_null_opt(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[TMP0:%.*]] = icmp eq i32 [[X:%.*]], 0
-; CHECK-NEXT:    br i1 [[TMP0]], label [[BB:%.*]], label [[RETURN:%.*]]
-; CHECK:       bb:
-; CHECK-NEXT:    [[TMP1:%.*]] = load volatile i32, i32* null, align 4
-; CHECK-NEXT:    unreachable
-; CHECK:       return:
+; CHECK-NEXT:    [[TMP1:%.*]] = xor i1 [[TMP0]], true
+; CHECK-NEXT:    call void @llvm.assume(i1 [[TMP1]])
 ; CHECK-NEXT:    ret void
 ;
 entry:
@@ -127,11 +121,8 @@
 define void @test5(i1 %C, i32* %P) {
 ; CHECK-LABEL: @test5(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    br i1 [[C:%.*]], label [[T:%.*]], label [[F:%.*]]
-; CHECK:       T:
-; CHECK-NEXT:    [[TMP0:%.*]] = cmpxchg volatile i32* [[P:%.*]], i32 0, i32 1 seq_cst seq_cst, align 4
-; CHECK-NEXT:    unreachable
-; CHECK:       F:
+; CHECK-NEXT:    [[TMP0:%.*]] = xor i1 [[C:%.*]], true
+; CHECK-NEXT:    call void @llvm.assume(i1 [[TMP0]])
 ; CHECK-NEXT:    ret void
 ;
 entry:
@@ -147,11 +138,8 @@
 define void @test6(i1 %C, i32* %P) {
 ; CHECK-LABEL: @test6(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    br i1 [[C:%.*]], label [[T:%.*]], label [[F:%.*]]
-; CHECK:       T:
-; CHECK-NEXT:    [[TMP0:%.*]] = atomicrmw volatile xchg i32* [[P:%.*]], i32 0 seq_cst, align 4
-; CHECK-NEXT:    unreachable
-; CHECK:       F:
+; CHECK-NEXT:    [[TMP0:%.*]] = xor i1 [[C:%.*]], true
+; CHECK-NEXT:    call void @llvm.assume(i1 [[TMP0]])
 ; CHECK-NEXT:    ret void
 ;
 entry:
Index: llvm/lib/Transforms/Utils/SimplifyCFG.cpp
===================================================================
--- llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -4673,17 +4673,13 @@
 
     if (BBI->mayHaveSideEffects()) {
       if (auto *SI = dyn_cast<StoreInst>(BBI)) {
+        // Temporarily disable removal of volatile stores preceding unreachable,
+        // pending a potential LangRef change permitting volatile stores to
+        // trap.
+        // TODO: Either remove this code, or properly integrate the check into
+        // isGuaranteedToTransferExecutionToSuccessor().
         if (SI->isVolatile())
           break;
-      } else if (auto *LI = dyn_cast<LoadInst>(BBI)) {
-        if (LI->isVolatile())
-          break;
-      } else if (auto *RMWI = dyn_cast<AtomicRMWInst>(BBI)) {
-        if (RMWI->isVolatile())
-          break;
-      } else if (auto *CXI = dyn_cast<AtomicCmpXchgInst>(BBI)) {
-        if (CXI->isVolatile())
-          break;
       } else if (isa<CatchPadInst>(BBI)) {
         // A catchpad may invoke exception object constructors and such, which
         // in some languages can be arbitrary code, so be conservative by
@@ -4692,8 +4688,9 @@
         if (classifyEHPersonality(BB->getParent()->getPersonalityFn()) !=
             EHPersonality::CoreCLR)
           break;
-      } else if (!isa<FenceInst>(BBI) && !isa<VAArgInst>(BBI) &&
-                 !isa<LandingPadInst>(BBI)) {
+      } else if (!isa<LoadInst>(BBI) && !isa<AtomicRMWInst>(BBI) &&
+                 !isa<AtomicCmpXchgInst>(BBI) && !isa<FenceInst>(BBI) &&
+                 !isa<VAArgInst>(BBI) && !isa<LandingPadInst>(BBI)) {
         break;
       }
       // Note that deleting LandingPad's here is in fact okay, although it


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D105343.356145.patch
Type: text/x-patch
Size: 4040 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210702/5786c48a/attachment.bin>