[MemoryDepndency] Relax the re-ordering with volatile store.

[skatkov]

Summary

Volatile store does not provide any special rules for reordering with atomics.
Usual must alias analysis is enough here.

This makes the behavior similar to how volatile load is handled.

[skatkov]

@reames , last time this part was touched (https://reviews.llvm.org/D16857, ooch 2016) you provided the meaningful review. I would appreciate if you could look at this at some point.

To be honest I do not have a motivation example for this change and probably it terms of "do not touch if it works" I can abandon this patch.
I just mentioned inconsistency in handling Load and Store and according to my understanding of spec there is no difference and decided it is better to improve it.

[nikic]

This looks correct to me, but I'm not an expert on volatile/atomic semantics.

[reames]

LGTM w/required changes pre-commit.

llvm/test/Analysis/MemoryDependenceAnalysis/reorder-volatile.ll

11

Autogen this please. And precommit the test before landing the change.

29

Please add a test with a volatile load wrapped between two acquires, and a separate one with it wrapped by seq_cst loads. They *should not* fold. (Well, with the current logic at least.)

[skatkov]

Handled comments. Will land soon.

llvm/lib/Analysis/MemoryDependenceAnalysis.cpp

if (StoreInst *SI = dyn_cast<StoreInst>(Inst)) {
       if (!SI->isUnordered() && SI->isAtomic()) {
         if (!QueryInst || isNonSimpleLoadOrStore(QueryInst) ||
             isOtherMemAccess(QueryInst))
           return MemDepResult::getClobber(SI);
         if (SI->getOrdering() != AtomicOrdering::Monotonic)
           return MemDepResult::getClobber(SI);
       }
 
-      // FIXME: this is overly conservative.
       // While volatile access cannot be eliminated, they do not have to clobber
       // non-aliasing locations, as normal accesses can for example be reordered
       // with volatile accesses.
       if (SI->isVolatile())
-        if (!QueryInst || isNonSimpleLoadOrStore(QueryInst) ||
-            isOtherMemAccess(QueryInst))
+        if (!QueryInst || QueryInst->isVolatile())
           return MemDepResult::getClobber(SI);
 
       // If alias analysis can tell that this store is guaranteed to not modify
       // the query pointer, ignore it.  Use getModRefInfo to handle cases where
       // the query pointer points to constant memory etc.
       if (!isModOrRefSet(BatchAA.getModRefInfo(SI, MemLoc)))
         continue;
 

llvm/test/Analysis/MemoryDependenceAnalysis/reorder-volatile.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt -S -gvn --basic-aa < %s | FileCheck %s
 
 @u = global i32 5, align 4
 @w = global i32 10, align 4
 
 define i32 @test_load() {
 ; CHECK-LABEL: @test_load(
 ; CHECK-NEXT:    [[LV:%.*]] = load volatile i32, i32* @u, align 4
 ; CHECK-NEXT:    ret i32 [[LV]]
 ;
   %l1 = load atomic i32, i32* @w unordered, align 4
   %lv = load volatile i32, i32* @u, align 4
   %l2 = load atomic i32, i32* @w unordered, align 4
   %res.1 = sub i32 %l1, %l2
   %res = add i32 %res.1, %lv
   ret i32 %res
 }
 
 define i32 @test_load_with_acquire_load() {
 ; CHECK-LABEL: @test_load_with_acquire_load(
 ; CHECK-NEXT:    [[L1:%.*]] = load atomic i32, i32* @w acquire, align 4
 ; CHECK-NEXT:    [[LV:%.*]] = load volatile i32, i32* @u, align 4
 ; CHECK-NEXT:    [[L2:%.*]] = load atomic i32, i32* @w acquire, align 4
 ; CHECK-NEXT:    [[RES_1:%.*]] = sub i32 [[L1]], [[L2]]
 ; CHECK-NEXT:    [[RES:%.*]] = add i32 [[RES_1]], [[LV]]
 ; CHECK-NEXT:    ret i32 [[RES]]
 ;
   %l1 = load atomic i32, i32* @w acquire, align 4
   %lv = load volatile i32, i32* @u, align 4
   %l2 = load atomic i32, i32* @w acquire, align 4
   %res.1 = sub i32 %l1, %l2
   %res = add i32 %res.1, %lv
   ret i32 %res
 }
 
 define i32 @test_load_with_seq_cst_load() {
;
   %l2 = load atomic i32, i32* @w seq_cst, align 4
   %res.1 = sub i32 %l1, %l2
   %res = add i32 %res.1, %lv
   ret i32 %res
 }
 
 define i32 @test_store(i32 %x) {
 ; CHECK-LABEL: @test_store(
-; CHECK-NEXT:    [[L1:%.*]] = load atomic i32, i32* @w unordered, align 4
 ; CHECK-NEXT:    store volatile i32 [[X:%.*]], i32* @u, align 4
-; CHECK-NEXT:    [[L2:%.*]] = load atomic i32, i32* @w unordered, align 4
-; CHECK-NEXT:    [[RES:%.*]] = sub i32 [[L1]], [[L2]]
-; CHECK-NEXT:    ret i32 [[RES]]
+; CHECK-NEXT:    ret i32 0
 ;
   %l1 = load atomic i32, i32* @w unordered, align 4
   store volatile i32 %x, i32* @u, align 4
   %l2 = load atomic i32, i32* @w unordered, align 4
   %res = sub i32 %l1, %l2
   ret i32 %res
 }