[LLVM IR] Allow volatile stores to trap.

[efriedma]

Summary

Proposed alternative to 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).

[jdoerfert]

I think this makes it consistent but I would have hoped to avoid it anyway. willreturn might not be the most important attribute yet but it's uses are growing. It affects align, dereferenceable, and already various optimizations already.

That said, the conversation in the other thread doesn't have to stop if we land this and this would at least give us some consistency, so I'm in favor of landing it.

[nikic]

llvm/lib/Analysis/ValueTracking.cpp

5295

This should be sunk into Instruction::willReturn(), otherwise FuncAttrs won't use the new semantics.

[efriedma]

Move check into Instruction::willReturn.

[lebedev.ri]

Note that this still won't quite match clang's expectations, since it expects that load volatile also may trap.

[nikic]

In D106309#2888872, @lebedev.ri wrote:

Note that this still won't quite match clang's expectations, since it expects that load volatile also may trap.

Where does clang expect that?

[lebedev.ri]

In D106309#2891403, @nikic wrote:

In D106309#2888872, @lebedev.ri wrote:

Note that this still won't quite match clang's expectations, since it expects that load volatile also may trap.

Where does clang expect that?

In the C++ specification, i believe.

[efriedma]

C++ [dcl.type.cv] explicitly states "The semantics of an access through a volatile glvalue are implementation-defined." I think that's a pretty clear statement that the standard wants to stay out of this debate. :)

[nikic]

LGTM, with the understanding that this is just bringing our current hacks into a coherent model, and does not match the semantics we actually want to have.

llvm/docs/LangRef.rst

 operation may modify the memory at that address. A volatile operation
 may not modify any other memory accessible by the module being compiled.
 A volatile operation may not call any code in the current module.
 
 The compiler may assume execution will continue after a volatile operation,
 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
 load/store instructions. Similarly, IR-level volatile loads and stores cannot
 change from integer to floating-point or vice versa.
 
 .. admonition:: Rationale
 

llvm/lib/Analysis/ValueTracking.cpp

bool llvm::isGuaranteedToTransferExecutionToSuccessor(const Instruction *I) {
   // arbitrary length of time, but programs aren't allowed to rely on that.
 
   // If there is no successor, then execution can't transfer to it.
   if (isa<ReturnInst>(I))
     return false;
   if (isa<UnreachableInst>(I))
     return false;
 
+  // Note: Do not add new checks here; instead, change Instruction::mayThrow or
+  // Instruction::willReturn.
+  //
+  // FIXME: Move this check into Instruction::willReturn.
   if (isa<CatchPadInst>(I)) {
     switch (classifyEHPersonality(I->getFunction()->getPersonalityFn())) {
     default:
       // A catchpad may invoke exception object constructors and such, which
       // in some languages can be arbitrary code, so be conservative by default.
       return false;
     case EHPersonality::CoreCLR:
       // For CoreCLR, it just involves a type test.
       return true;
     }
   }
 
   // An instruction that returns without throwing must transfer control flow
   // to a successor.
   return !I->mayThrow() && I->willReturn();
 }
 
 bool llvm::isGuaranteedToTransferExecutionToSuccessor(const BasicBlock *BB) {
   // TODO: This is slightly conservative for invoke instruction since exiting
   // via an exception *is* normal control for them.
   for (const Instruction &I : *BB)
     if (!isGuaranteedToTransferExecutionToSuccessor(&I))
       return false;

llvm/lib/IR/Instruction.cpp

 }
 
 bool Instruction::isSafeToRemove() const {
   return (!isa<CallInst>(this) || !this->mayHaveSideEffects()) &&
          !this->isTerminator();
 }
 
 bool Instruction::willReturn() const {
+  // Volatile store isn't guaranteed to return; see LangRef.
+  if (auto *SI = dyn_cast<StoreInst>(this))
+    return !SI->isVolatile();
+
   if (const auto *CB = dyn_cast<CallBase>(this))
     // FIXME: Temporarily assume that all side-effect free intrinsics will
     // return. Remove this workaround once all intrinsics are appropriately
     // annotated.
     return CB->hasFnAttr(Attribute::WillReturn) ||
            (isa<IntrinsicInst>(CB) && CB->onlyReadsMemory());
   return true;
 }

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp

while (Instruction *Prev = I.getPrevNonDebugInstruction()) {
     if (Prev->isEHPad())
       return nullptr; // Can not drop any more instructions. We're done here.
 
     if (!isGuaranteedToTransferExecutionToSuccessor(Prev))
       return nullptr; // Can not drop any more instructions. We're done here.
     // 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()));
     eraseInstFromFunction(*Prev);
   }
   assert(I.getParent()->sizeWithoutDebug() == 1 && "The block is now empty.");
   // FIXME: recurse into unconditional predecessors?
   return nullptr;

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

while (UI->getIterator() != BB->begin()) {
     BasicBlock::iterator BBI = UI->getIterator();
     --BBI;
 
     if (!isGuaranteedToTransferExecutionToSuccessor(&*BBI))
       break; // Can not drop any more instructions. We're done here.
     // 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,
     // and we can therefore guarantee this block will be erased.
 
     // Delete this instruction (any uses are guaranteed to be dead)
     BBI->replaceAllUsesWith(PoisonValue::get(BBI->getType()));
     BBI->eraseFromParent();

llvm/test/Transforms/FunctionAttrs/nosync.ll

;
   store atomic i32 10, i32* %0 unordered, align 4
   ret void
 }
 
 
 ; negative, should not deduce nosync
 ; atomic load with release ordering
 define void @load_release(i32* nocapture %0) norecurse nounwind uwtable {
-; CHECK: Function Attrs: mustprogress nofree norecurse nounwind uwtable willreturn
+; CHECK: Function Attrs: nofree norecurse nounwind uwtable
 ; CHECK-LABEL: @load_release(
 ; CHECK-NEXT:    store atomic volatile i32 10, i32* [[TMP0:%.*]] release, align 4
 ; CHECK-NEXT:    ret void
 ;
   store atomic volatile i32 10, i32* %0 release, align 4
   ret void
 }
 
 ; negative volatile, relaxed atomic
 define void @load_volatile_release(i32* nocapture %0) norecurse nounwind uwtable {
-; CHECK: Function Attrs: mustprogress nofree norecurse nounwind uwtable willreturn
+; CHECK: Function Attrs: nofree norecurse nounwind uwtable
 ; CHECK-LABEL: @load_volatile_release(
 ; CHECK-NEXT:    store atomic volatile i32 10, i32* [[TMP0:%.*]] release, align 4
 ; CHECK-NEXT:    ret void
 ;
   store atomic volatile i32 10, i32* %0 release, align 4
   ret void
 }
 
 ; volatile store.
 define void @volatile_store(i32* %0) norecurse nounwind uwtable {
-; CHECK: Function Attrs: mustprogress nofree norecurse nounwind uwtable willreturn
+; CHECK: Function Attrs: nofree norecurse nounwind uwtable
 ; CHECK-LABEL: @volatile_store(
 ; CHECK-NEXT:    store volatile i32 14, i32* [[TMP0:%.*]], align 4
 ; CHECK-NEXT:    ret void
 ;
   store volatile i32 14, i32* %0, align 4
   ret void
 }
 

llvm/test/Transforms/LICM/sink-debuginfo-preserve.ll

bb13:                                             ; preds = %bb17, %bb7
   br i1 %i15, label %bb16, label %bb17, !dbg !55
 
 bb16:                                             ; preds = %bb13
   store i32 0, i32* getelementptr inbounds ([2 x i32], [2 x i32]* @g_390, i64 0, i64 1), align 4, !dbg !56, !tbaa !40
   br label %bb17
 
 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
 
 bb22:                                             ; preds = %bb17
   br label %bb23, !dbg !48
 
 bb23:                                             ; preds = %bb22, %bb4

llvm/test/Transforms/PhaseOrdering/inlining-alignment-assumptions.ll

 ; ASSUMPTIONS-OFF-NEXT:    ret void
 ; ASSUMPTIONS-OFF:       false2:
 ; ASSUMPTIONS-OFF-NEXT:    store volatile i64 1, i64* [[PTR]], align 4
 ; ASSUMPTIONS-OFF-NEXT:    br label [[COMMON_RET]]
 ;
 ; 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:%.*]] ]
 ; ASSUMPTIONS-ON-NEXT:    call void @llvm.assume(i1 true) [ "align"(i64* [[PTR]], i64 8) ]
 ; ASSUMPTIONS-ON-NEXT:    store volatile i64 0, i64* [[PTR]], align 8
 ; ASSUMPTIONS-ON-NEXT:    store volatile i64 -1, i64* [[PTR]], align 8
 ; ASSUMPTIONS-ON-NEXT:    store volatile i64 -1, i64* [[PTR]], align 8
 ; ASSUMPTIONS-ON-NEXT:    store volatile i64 -1, i64* [[PTR]], align 8