[ValueTracking] Remove volatile check in isGuaranteedToTransferExecutionToSuccessor

[uenoku]

Summary

As clarified in D53184, volatile load and store do not trap. Therefore, we should remove volatile checks for instructions in isGuaranteedToTransferExecutionToSuccessor.

[nikic]

LGTM per langref:

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.

llvm/lib/Analysis/ValueTracking.cpp

4268 ↗

(On Diff #212091)

Side note: These special-cased intrinsics should be synced now that willreturn exists. In particular assume can be removed from the list because it is already willreturn. sideeffect and experimental.widenable.condition should be marked as willreturn (they must be, otherwise this code would be incorrect) and then removed from here.

llvm/test/Transforms/FunctionAttrs/nonnull.ll

350 ↗

(On Diff #212091)

nit: wouldn't -> can't

[uenoku]

Address comment.

[uenoku]

llvm/lib/Analysis/ValueTracking.cpp

4268 ↗

(On Diff #212091)

I agree. I'll fix it in another patch.

llvm/trunk/lib/Analysis/ValueTracking.cpp

OverflowResult llvm::computeOverflowForSignedAdd(const Value *LHS,
                                                  const DataLayout &DL,
                                                  AssumptionCache *AC,
                                                  const Instruction *CxtI,
                                                  const DominatorTree *DT) {
   return ::computeOverflowForSignedAdd(LHS, RHS, nullptr, DL, AC, CxtI, DT);
 }
 
 bool llvm::isGuaranteedToTransferExecutionToSuccessor(const Instruction *I) {
-  // A memory operation returns normally if it isn't volatile. A volatile
-  // operation is allowed to trap.
-  //
-  // An atomic operation isn't guaranteed to return in a reasonable amount of
-  // time because it's possible for another thread to interfere with it for an
+  // Note: An atomic operation isn't guaranteed to return in a reasonable amount
+  // of time because it's possible for another thread to interfere with it for an
   // arbitrary length of time, but programs aren't allowed to rely on that.
-  if (const LoadInst *LI = dyn_cast<LoadInst>(I))
-    return !LI->isVolatile();
-  if (const StoreInst *SI = dyn_cast<StoreInst>(I))
-    return !SI->isVolatile();
-  if (const AtomicCmpXchgInst *CXI = dyn_cast<AtomicCmpXchgInst>(I))
-    return !CXI->isVolatile();
-  if (const AtomicRMWInst *RMWI = dyn_cast<AtomicRMWInst>(I))
-    return !RMWI->isVolatile();
-  if (const MemIntrinsic *MII = dyn_cast<MemIntrinsic>(I))
-    return !MII->isVolatile();
 
   // If there is no successor, then execution can't transfer to it.
   if (const auto *CRI = dyn_cast<CleanupReturnInst>(I))
     return !CRI->unwindsToCaller();
   if (const auto *CatchSwitch = dyn_cast<CatchSwitchInst>(I))
     return !CatchSwitch->unwindsToCaller();
   if (isa<ResumeInst>(I))
     return false;

llvm/trunk/test/Transforms/FunctionAttrs/nonnull.ll

 t:
   call void @use1nonnull(i8* %a)
   ret void
 f:
   ret void
 }
 
 ; The callsite must execute in order for the attribute to transfer to the parent.
-; The volatile load might trap, so there's no guarantee that we'll ever get to the call.
+; The volatile load can't trap, so we can guarantee that we'll get to the call.
 
 define i8 @parent6(i8* %a, i8* %b) {
-; BOTH-LABEL: @parent6(i8* %a, i8* %b)
+; FNATTR-LABEL: @parent6(i8* nonnull %a, i8* %b)
+; FIXME: missing "nonnull"
+; ATTRIBUTOR-LABEL: @parent6(i8* %a, i8* %b)
 ; BOTH-NEXT:    [[C:%.*]] = load volatile i8, i8* %b
 ; FNATTR-NEXT:    call void @use1nonnull(i8* %a)
 ; ATTRIBUTOR-NEXT:    call void @use1nonnull(i8* nonnull %a)
 ; BOTH-NEXT:    ret i8 [[C]]
 
   %c = load volatile i8, i8* %b
   call void @use1nonnull(i8* %a)
   ret i8 %c