[SimplifyCFG] Volatile memory operations do not trap

[lebedev.ri]

Summary

Somewhat related to 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

[nikic]

This whole code doesn't really make sense. Can we switch it to isGuaranteedToTransferExecutionToSuccessor() (plus the volatile store exception)?

[lebedev.ri]

In D105343#2855407, @nikic wrote:

This whole code doesn't really make sense. Can we switch it to isGuaranteedToTransferExecutionToSuccessor() (plus the volatile store exception)?

Yep, this code is really broken.
I'm intentionally not just doing that because i want to make the test diff smaller. (i.e. isGuaranteedToTransferExecutionToSuccessor() will be either next patch, or one after that)

[nikic]

Okay, LGTM then. This converges on guaranteed-to-execute, and InstCombine has been doing this for quite a while now, so it's clearly fine in practice as well.

[lebedev.ri]

Yep.
Thank you for the review.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

while (UI->getIterator() != BB->begin()) {
     // Do not delete instructions that can have side effects which might cause
     // the unreachable to not be reachable; specifically, calls and volatile
     // operations may have this effect.
     if (isa<CallInst>(BBI) && !isa<DbgInfoIntrinsic>(BBI))
       break;
 
     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
         // default.
         // For CoreCLR, it just involves a type test, so can be removed.
         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
       // involves a bit of subtle reasoning. If this inst is a LandingPad,
       // all the predecessors of this block will be the unwind edges of Invokes,
       // and we can therefore guarantee this block will be erased.
     }
 

llvm/test/Transforms/SimplifyCFG/trapping-load-unreachable.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -simplifycfg -simplifycfg-require-and-preserve-domtree=1 -S | FileCheck %s
 ; PR2967
 
 target datalayout =
 "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32"
 target triple = "i386-pc-linux-gnu"
 
 define void @test1(i32 %x) nounwind {
 ; 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:
   %0 = icmp eq i32 %x, 0          ; <i1> [#uses=1]
   br i1 %0, label %bb, label %return
 
 bb:             ; preds = %entry
   %1 = load volatile i32, i32* null
   unreachable
 
   br label %return
 return:         ; preds = %entry
   ret void
 }
 
 define void @test1_no_null_opt(i32 %x) nounwind #0 {
 ; 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:
   %0 = icmp eq i32 %x, 0          ; <i1> [#uses=1]
   br i1 %0, label %bb, label %return
 
 bb:             ; preds = %entry
   %1 = load volatile i32, i32* null

 F:
   ret void
 }
 
 ; Check cmpxchg before unreachable.
 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:
   br i1 %C, label %T, label %F
 T:
   cmpxchg volatile i32* %P, i32 0, i32 1 seq_cst seq_cst
   unreachable
 F:
   ret void
 }
 
 ; Check atomicrmw before unreachable.
 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:
   br i1 %C, label %T, label %F
 T:
   atomicrmw volatile xchg i32* %P, i32 0 seq_cst
   unreachable
 F:
   ret void
 }
 
 attributes #0 = { null_pointer_is_valid }