[PATCH] D105338: [InstCombine] Revert "Temporarily do not drop volatile stores before unreachable"

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri updated this revision to Diff 357272.
lebedev.ri added a comment.

Rebased.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105338/new/

https://reviews.llvm.org/D105338

Files:
  llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
  llvm/lib/Transforms/Utils/Local.cpp
  llvm/lib/Transforms/Utils/SimplifyCFG.cpp
  llvm/test/Transforms/InstCombine/volatile_store.ll
  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
@@ -76,8 +76,8 @@
 define void @test3() nounwind {
 ; CHECK-LABEL: @test3(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    store volatile i32 4, i32* null, align 4
-; CHECK-NEXT:    ret void
+; CHECK-NEXT:    call void @llvm.trap()
+; CHECK-NEXT:    unreachable
 ;
 entry:
   store volatile i32 4, i32* null
@@ -101,11 +101,8 @@
 define void @test4(i1 %C, i32* %P) {
 ; CHECK-LABEL: @test4(
 ; CHECK-NEXT:  entry:
-; CHECK-NEXT:    br i1 [[C:%.*]], label [[T:%.*]], label [[F:%.*]]
-; CHECK:       T:
-; CHECK-NEXT:    store volatile i32 0, i32* [[P:%.*]], 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/test/Transforms/InstCombine/volatile_store.ll
===================================================================
--- llvm/test/Transforms/InstCombine/volatile_store.ll
+++ llvm/test/Transforms/InstCombine/volatile_store.ll
@@ -25,7 +25,6 @@
 ; CHECK-LABEL: @volatile_store_before_unreachable(
 ; CHECK-NEXT:    br i1 [[C:%.*]], label [[TRUE:%.*]], label [[FALSE:%.*]]
 ; CHECK:       true:
-; CHECK-NEXT:    store volatile i8 0, i8* [[P:%.*]], align 1
 ; CHECK-NEXT:    unreachable
 ; CHECK:       false:
 ; CHECK-NEXT:    ret void
Index: llvm/lib/Transforms/Utils/SimplifyCFG.cpp
===================================================================
--- llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -4672,14 +4672,6 @@
     // Otherwise, this instruction can be freely erased,
     // even if it is not side-effect free.
 
-    // 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 (auto *SI = dyn_cast<StoreInst>(&*BBI))
-      if (SI->isVolatile())
-        break; // Can not drop this instruction. We're done here.
-
     // Note that deleting EH's here is in fact okay, although it involves a bit
     // of subtle reasoning. If this inst is an EH, all the predecessors of this
     // block will be the unwind edges of Invoke/CatchSwitch/CleanupReturn,
Index: llvm/lib/Transforms/Utils/Local.cpp
===================================================================
--- llvm/lib/Transforms/Utils/Local.cpp
+++ llvm/lib/Transforms/Utils/Local.cpp
@@ -2297,9 +2297,6 @@
         // that they should be changed to unreachable by passes that can't
         // modify the CFG.
 
-        // Don't touch volatile stores.
-        if (SI->isVolatile()) continue;
-
         Value *Ptr = SI->getOperand(1);
 
         if (isa<UndefValue>(Ptr) ||
Index: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
===================================================================
--- llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -2888,14 +2888,6 @@
     // Otherwise, this instruction can be freely erased,
     // even if it is not side-effect free.
 
-    // 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 (auto *SI = dyn_cast<StoreInst>(Prev))
-      if (SI->isVolatile())
-        return nullptr; // Can not drop this instruction. We're done here.
-
     // A value may still have uses before we process it here (for example, in
     // another unreachable block), so convert those to poison.
     replaceInstUsesWith(*Prev, PoisonValue::get(Prev->getType()));


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D105338.357272.patch
Type: text/x-patch
Size: 4092 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210708/27558053/attachment.bin>