[PATCH] D106309: [LLVM IR] Allow volatile stores to trap.

[Eli Friedman via Phabricator via llvm-commits]

efriedma created this revision.
efriedma added reviewers: lebedev.ri, nikic, nlopes, jdoerfert, jfb, chandlerc, spatel, kparzysz, thakis, rsmith, xbolva00.
Herald added subscribers: asbirlea, hiraditya.
efriedma requested review of this revision.
Herald added a project: LLVM.

Proposed alternative to D105338 <https://reviews.llvm.org/D105338>.

This is ugly, but short-term I think it's the best way forward: first, let's formalize the hacks into a coherent model. Then we can consider extensions of that model (we could have different flavors of volatile with different rules).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106309

Files:
  llvm/docs/LangRef.rst
  llvm/lib/Analysis/ValueTracking.cpp
  llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
  llvm/lib/Transforms/Utils/SimplifyCFG.cpp
  llvm/test/Transforms/LICM/sink-debuginfo-preserve.ll
  llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll


Index: llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll
===================================================================
--- llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll
+++ llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll
@@ -34,7 +34,7 @@
 ; ASSUMPTIONS-ON-LABEL: @caller1(
 ; ASSUMPTIONS-ON-NEXT:    br i1 [[C:%.*]], label [[COMMON_RET:%.*]], label [[FALSE1:%.*]]
 ; ASSUMPTIONS-ON:       false1:
-; ASSUMPTIONS-ON-NEXT:    store volatile i64 1, i64* [[PTR:%.*]], align 8
+; ASSUMPTIONS-ON-NEXT:    store volatile i64 1, i64* [[PTR:%.*]], align 4
 ; ASSUMPTIONS-ON-NEXT:    br label [[COMMON_RET]]
 ; ASSUMPTIONS-ON:       common.ret:
 ; ASSUMPTIONS-ON-NEXT:    [[DOTSINK:%.*]] = phi i64 [ 3, [[FALSE1]] ], [ 2, [[TMP0:%.*]] ]
Index: llvm/test/Transforms/LICM/sink-debuginfo-preserve.ll
===================================================================
--- llvm/test/Transforms/LICM/sink-debuginfo-preserve.ll
+++ llvm/test/Transforms/LICM/sink-debuginfo-preserve.ll
@@ -68,8 +68,6 @@
 
 bb17:                                             ; preds = %bb16, %bb13
   %i18 = load volatile i32, i32* @c, align 4, !dbg !57, !tbaa !40
-  %i19 = add nsw i32 %i18, 1, !dbg !57
-  store volatile i32 %i19, i32* @c, align 4, !dbg !57, !tbaa !40
   %i20 = load volatile i32, i32* @c, align 4, !dbg !46, !tbaa !40
   %i21 = icmp slt i32 %i20, 6, !dbg !47
   br i1 %i21, label %bb13, label %bb22, !dbg !48, !llvm.loop !58
Index: llvm/lib/Transforms/Utils/SimplifyCFG.cpp
===================================================================
--- llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -4674,14 +4674,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/InstCombine/InstructionCombining.cpp
===================================================================
--- llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
+++ llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
@@ -2912,14 +2912,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()));
Index: llvm/lib/Analysis/ValueTracking.cpp
===================================================================
--- llvm/lib/Analysis/ValueTracking.cpp
+++ llvm/lib/Analysis/ValueTracking.cpp
@@ -5286,6 +5286,10 @@
     }
   }
 
+  // Volatile store isn't guaranteed to return; see LangRef.
+  if (auto *SI = dyn_cast<StoreInst>(I))
+    return !SI->isVolatile();
+
   // An instruction that returns without throwing must transfer control flow
   // to a successor.
   return !I->mayThrow() && I->willReturn();
Index: llvm/docs/LangRef.rst
===================================================================
--- llvm/docs/LangRef.rst
+++ llvm/docs/LangRef.rst
@@ -2861,6 +2861,12 @@
 so operations which modify memory or may have undefined behavior can be
 hoisted past a volatile operation.
 
+As an exception to the preceding rule, the compiler may not assume execution
+will continue after a volatile store operation. This restriction is necessary
+to support the somewhat common pattern in C of intentionally storing to an
+invalid pointer to crash the program. In the future, it might make sense to
+allow frontends to control this behavior.
+
 IR-level volatile loads and stores cannot safely be optimized into llvm.memcpy
 or llvm.memmove intrinsics even when those intrinsics are flagged volatile.
 Likewise, the backend should never split or merge target-legal volatile


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D106309.359902.patch
Type: text/x-patch
Size: 4898 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210719/d9672148/attachment.bin>