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

[Roman Lebedev via Phabricator via llvm-commits]

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

This reverts commit 4e413e16216d0c94ada2171f3c59e0a85f4fa4b6 <https://reviews.llvm.org/rG4e413e16216d0c94ada2171f3c59e0a85f4fa4b6>,
which landed almost 10 months ago under premise that the original behavior
didn't match reality and was breaking users, even though it was correct as per
the LangRef. But the LangRef change still hasn't appeared, which might suggest
that the affected parties aren't really worried about this problem.

Perhaps, this isn't destined to land, but merely to nudge towards further disscussion and a proper resolution.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105338

Files:
  llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
  llvm/test/Transforms/InstCombine/volatile_store.ll


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/InstCombine/InstructionCombining.cpp
===================================================================
--- llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -2877,14 +2877,6 @@
   Instruction *Prev = I.getPrevNonDebugInstruction();
   if (Prev && !Prev->isEHPad() &&
       isGuaranteedToTransferExecutionToSuccessor(Prev)) {
-    // 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;
-
     // 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.356130.patch
Type: text/x-patch
Size: 1572 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210702/18b4604f/attachment.bin>