[AliasAnalysis] Fences do not modify constant memory location

[anna]

Summary

Fence instructions are currently marked as ModRef for all memory locations.

We can improve this for constant memory locations (such as constant globals),
since fence instructions cannot modify these locations.

This helps us to forward constant loads across fences (added test case in GVN).
There were no changes in behaviour for similar test cases in early-cse and licm.

[• dberlin]

Even though most AA related calls will crash trying to get a memorylocation from the fence, this looks right..

Please add the same change to NewGVN/fence.ll

[anna]

Added test case to NewGVN as well.

[anna]

Hi Daniel,

In D28914#650775, @dberlin wrote:

Even though most AA related calls will crash trying to get a memorylocation from the fence, this looks right..

I've seen the lack of memory locations in optimizations when using memssa version for optimizations. So, in such cases, it would still return ModRef.

[• dberlin]

Somewhere on my todo list is to cleanup getModRefInfo, and make getModRefInfo the only interface memssa uses.
Right now, it has to differentiate between calls, fences, and other things to call the right versions.

Interestingly, fences are the only memory modification instruction we have that has no location, and if you call memorylocation::get on them, we crash.

llvm/trunk/include/llvm/Analysis/AliasAnalysis.h

public:
 
   /// getModRefInfo (for stores) - A convenience wrapper.
   ModRefInfo getModRefInfo(const StoreInst *S, const Value *P, uint64_t Size) {
     return getModRefInfo(S, MemoryLocation(P, Size));
   }
 
   /// getModRefInfo (for fences) - Return information about whether
   /// a particular store modifies or reads the specified memory location.
-  ModRefInfo getModRefInfo(const FenceInst *S, const MemoryLocation &Loc) {
-    // Conservatively correct.  (We could possibly be a bit smarter if
-    // Loc is a alloca that doesn't escape.)
-    return MRI_ModRef;
-  }
+  ModRefInfo getModRefInfo(const FenceInst *S, const MemoryLocation &Loc);
 
   /// getModRefInfo (for fences) - A convenience wrapper.
   ModRefInfo getModRefInfo(const FenceInst *S, const Value *P, uint64_t Size) {
     return getModRefInfo(S, MemoryLocation(P, Size));
   }
 
   /// getModRefInfo (for cmpxchges) - Return information about whether
   /// a particular cmpxchg modifies or reads the specified memory location.

llvm/trunk/lib/Analysis/AliasAnalysis.cpp

if (Loc.Ptr) {
     if (pointsToConstantMemory(Loc))
       return MRI_NoModRef;
   }
 
   // Otherwise, a store just writes.
   return MRI_Mod;
 }
 
+ModRefInfo AAResults::getModRefInfo(const FenceInst *S, const MemoryLocation &Loc) {
+  // If we know that the location is a constant memory location, the fence
+  // cannot modify this location.
+  if (Loc.Ptr && pointsToConstantMemory(Loc))
+    return MRI_Ref;
+  return MRI_ModRef;
+}
+
 ModRefInfo AAResults::getModRefInfo(const VAArgInst *V,
                                     const MemoryLocation &Loc) {
 
   if (Loc.Ptr) {
     // If the va_arg address cannot alias the pointer in question, then the
     // specified memory cannot be accessed by the va_arg.
     if (!alias(MemoryLocation::get(V), Loc))
       return MRI_NoModRef;

llvm/trunk/test/Transforms/GVN/fence.ll

 ; RUN: opt -S -basicaa -gvn < %s | FileCheck %s
 
+@a = external constant i32
 ; We can value forward across the fence since we can (semantically) 
 ; reorder the following load before the fence.
 define i32 @test(i32* %addr.i) {
 ; CHECK-LABEL: @test
 ; CHECK: store
 ; CHECK: fence
 ; CHECK-NOT: load
 ; CHECK: ret
; CHECK: ret i32 %res
   fence acquire
   %a = load i32, i32* %addr.i, align 4
   fence acquire
   %a2 = load i32, i32* %addr.i, align 4
   %res = sub i32 %a, %a2
   ret i32 %res
 }
 
+; We can forward the value forward the load
+; across both the fences, because the load is from
+; a constant memory location.
+define i32 @test4(i32* %addr) {
+; CHECK-LABEL: @test4
+; CHECK-NOT: load
+; CHECK: fence release
+; CHECK: store
+; CHECK: fence seq_cst
+; CHECK: ret i32 0
+  %var = load i32, i32* @a
+  fence release
+  store i32 42, i32* %addr, align 8
+  fence seq_cst
+  %var2 = load i32, i32* @a
+  %var3 = sub i32 %var, %var2
+  ret i32 %var3
+}
+
 ; Another example of why forwarding across an acquire fence is problematic
 ; can be seen in a normal locking operation.  Say we had:
 ; *p = 5; unlock(l); lock(l); use(p);
 ; forwarding the store to p would be invalid.  A reasonable implementation
 ; of unlock and lock might be:
 ; unlock() { atomicrmw sub %l, 1 unordered; fence release }
 ; lock() { 
 ;   do {
 ;     %res = cmpxchg %p, 0, 1, monotonic monotonic
 ;   } while(!%res.success)
 ;   fence acquire;
 ; }
 ; Given we chose to forward across the release fence, we clearly can't forward
 ; across the acquire fence as well.
 

llvm/trunk/test/Transforms/NewGVN/fence.ll

 ; XFAIL: *
 ; RUN: opt -S -basicaa -newgvn < %s | FileCheck %s
 
+@a = external constant i32
 ; We can value forward across the fence since we can (semantically) 
 ; reorder the following load before the fence.
 define i32 @test(i32* %addr.i) {
 ; CHECK-LABEL: @test
 ; CHECK: store
 ; CHECK: fence
 ; CHECK-NOT: load
 ; CHECK: ret
; CHECK: ret i32 %res
   fence acquire
   %a = load i32, i32* %addr.i, align 4
   fence acquire
   %a2 = load i32, i32* %addr.i, align 4
   %res = sub i32 %a, %a2
   ret i32 %res
 }
 
+; We can forward the value forward the load
+; across both the fences, because the load is from
+; a constant memory location.
+define i32 @test4(i32* %addr) {
+; CHECK-LABEL: @test4
+; CHECK-NOT: load
+; CHECK: fence release
+; CHECK: store
+; CHECK: fence seq_cst
+; CHECK: ret i32 0
+  %var = load i32, i32* @a
+  fence release
+  store i32 42, i32* %addr, align 8
+  fence seq_cst
+  %var2 = load i32, i32* @a
+  %var3 = sub i32 %var, %var2
+  ret i32 %var3
+}
+
 ; Another example of why forwarding across an acquire fence is problematic
 ; can be seen in a normal locking operation.  Say we had:
 ; *p = 5; unlock(l); lock(l); use(p);
 ; forwarding the store to p would be invalid.  A reasonable implementation
 ; of unlock and lock might be:
 ; unlock() { atomicrmw sub %l, 1 unordered; fence release }
 ; lock() { 
 ;   do {
 ;     %res = cmpxchg %p, 0, 1, monotonic monotonic
 ;   } while(!%res.success)
 ;   fence acquire;
 ; }
 ; Given we chose to forward across the release fence, we clearly can't forward
 ; across the acquire fence as well.