AMDGPU: If a store defines (alias) a load, it clobbers the load.

[cfang]

Summary

If a store defines (must alias) a load, it clobbers the load.

Fixes: SWDEV-258915

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUAnnotateUniformValues.cpp

115

Specifically checking for store instructions is going to be wrong since many other instruction types can also write to memory

[cfang]

Change store to any instructions that may write to memory.

[cfang]

llvm/lib/Target/AMDGPU/AMDGPUAnnotateUniformValues.cpp

115

Changed to mayWriteToMemory.

[arsenm]

llvm/test/CodeGen/AMDGPU/store-clobbers-load.ll

23

Should add some tests with exotic memory writes since this was bug before

[cfang]

llvm/test/CodeGen/AMDGPU/store-clobbers-load.ll

23

The memory dependence analysis will return isClobber (other than isDef) dependence for the "exotic memory writes".
While such tests maybe useful, they are beyond this patch.

[cfang]

Add a new test case.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUAnnotateUniformValues.cpp

115

Why does it need to check the instruction at all? Why isn't Q.isDef() sufficient?

[cfang]

llvm/lib/Target/AMDGPU/AMDGPUAnnotateUniformValues.cpp

115

Q.isDef() does not mean there must be a dependence at all.
One example is AllocaInst, in which case isDef() is returned, but the load could be optimized to undef.
I think instructional check of mayWriteToMemory is necessary here.

[arsenm]

llvm/test/CodeGen/AMDGPU/store-clobbers-load.ll

16

Why are we bothering to annotate a private memory access in the first place?

[cfang]

llvm/test/CodeGen/AMDGPU/store-clobbers-load.ll

16

This is a good question. Just found that the only place that uses amdgpu.noclobber metadata is
in SITargetLowering::LowerLOAD for global address space.

Should we limit the analysis only to global address space (I am not clear whether there will be future
usage of amdgpu.noclobber)?

llvm/lib/Target/AMDGPU/AMDGPUAnnotateUniformValues.cpp

   }
 
   DFS(Start, Checklist);
   for (auto &BB : Checklist) {
     BasicBlock::iterator StartIt = (!L && (BB == Load->getParent())) ?
       BasicBlock::iterator(Load) : BB->end();
     auto Q = MDR->getPointerDependencyFrom(
         MemoryLocation::getBeforeOrAfter(Ptr), true, StartIt, BB, Load);
-    if (Q.isClobber() || Q.isUnknown())
+    if (Q.isClobber() || Q.isUnknown() ||
+        // Store defines the load and thus clobbers it.
+        (Q.isDef() && Q.getInst()->mayWriteToMemory()))
       return true;
   }
   return false;
 }
 
 void AMDGPUAnnotateUniformValues::visitBranchInst(BranchInst &I) {
   if (DA->isUniform(&I))
     setUniformMetadata(I.getParent()->getTerminator());

llvm/test/CodeGen/AMDGPU/store-clobbers-load.ll

+; RUN: opt -S --amdgpu-annotate-uniform < %s | FileCheck -check-prefix=OPT %s
+target datalayout = "A5"
+
+; "load vaddr" depends on the store, so we should not mark vaddr as amdgpu.noclobber.
+
+; OPT-LABEL: @store_clobbers_load(
+; OPT:      %vaddr = bitcast [4 x i32] addrspace(5)* %alloca to <4 x i32> addrspace(5)*, !amdgpu.uniform !0
+; OPT-NEXT: %zero = load <4 x i32>, <4 x i32> addrspace(5)* %vaddr, align 16
+define amdgpu_kernel void @store_clobbers_load(i32 addrspace(1)* %out, i32 %index) {
+entry:
+  %alloca = alloca [4 x i32], addrspace(5)
+  %addr0 = bitcast [4 x i32] addrspace(5)* %alloca to i32 addrspace(5)*
+  store i32 0, i32 addrspace(5)* %addr0
+  %vaddr = bitcast [4 x i32] addrspace(5)* %alloca to <4 x i32> addrspace(5)*
+  %zero = load <4 x i32>, <4 x i32> addrspace(5)* %vaddr, align 16
+  %one = insertelement <4 x i32> %zero, i32 1, i32 1
+  %two = insertelement <4 x i32> %one, i32 2, i32 2
+  %three = insertelement <4 x i32> %two, i32 3, i32 3
+  store <4 x i32> %three, <4 x i32> addrspace(5)* %vaddr, align 16
+  %rslt = extractelement <4 x i32> %three, i32 %index
+  store i32 %rslt, i32 addrspace(1)* %out, align 4
+  ret void
+}

llvm/test/CodeGen/AMDGPU/wave32.ll

 
 ; GCN-LABEL: {{^}}test_loop_with_if_else_break:
 ; GFX1032: s_and_saveexec_b32 s{{[0-9]+}}, vcc_lo
 ; GFX1064: s_and_saveexec_b64 s[{{[0-9:]+}}], vcc{{$}}
 ; GCN:     s_cbranch_execz
 ; GCN:   ; %bb.{{[0-9]+}}: ; %.preheader
 ; GCN:   BB{{.*}}:
 
+; GCN:     global_store_dword
 ; GFX1032: s_or_b32 [[MASK0:s[0-9]+]], [[MASK0]], vcc_lo
 ; GFX1064: s_or_b64 [[MASK0:s\[[0-9:]+\]]], [[MASK0]], vcc
-; GCN:     global_store_dword
 ; GFX1032: s_andn2_b32 [[MASK1:s[0-9]+]], [[MASK1]], exec_lo
 ; GFX1064: s_andn2_b64 [[MASK1:s\[[0-9:]+\]]], [[MASK1]], exec
 ; GFX1032: s_and_b32 [[MASK0]], [[MASK0]], exec_lo
 ; GFX1064: s_and_b64 [[MASK0]], [[MASK0]], exec
 ; GFX1032: s_or_b32 [[MASK1]], [[MASK1]], [[MASK0]]
 ; GFX1064: s_or_b64 [[MASK1]], [[MASK1]], [[MASK0]]
 ; GCN:   BB{{.*}}: ; %Flow
 ; GFX1032: s_and_b32 [[TMP0:s[0-9]+]], exec_lo, [[MASK1]]
 ; GFX1064: s_and_b64 [[TMP0:s\[[0-9:]+\]]], exec, [[MASK1]]
 ; GFX1032: s_or_b32  [[ACC:s[0-9]+]], [[TMP0]], [[ACC]]
 ; GFX1064: s_or_b64  [[ACC:s\[[0-9:]+\]]], [[TMP0]], [[ACC]]
 ; GFX1032: s_andn2_b32 exec_lo, exec_lo, [[ACC]]
 ; GFX1064: s_andn2_b64 exec, exec, [[ACC]]
 ; GCN:     s_cbranch_execz
 ; GCN:   BB{{.*}}:
-; GCN: s_load_dword [[LOAD:s[0-9]+]]
+
 ; GFX1032: s_or_b32 [[MASK1]], [[MASK1]], exec_lo
 ; GFX1064: s_or_b64 [[MASK1]], [[MASK1]], exec
-; GCN: s_cmp_lt_i32 [[LOAD]], 11
+; GCN: global_load_dword [[LOAD:v[0-9]+]]
+; GFX1032: v_cmp_gt_i32_e32 vcc_lo, 11, [[LOAD]]
+; GFX1064: v_cmp_gt_i32_e32 vcc, 11, [[LOAD]]
 define amdgpu_kernel void @test_loop_with_if_else_break(i32 addrspace(1)* %arg) #0 {
 bb:
   %tmp = tail call i32 @llvm.amdgcn.workitem.id.x()
   %tmp1 = icmp eq i32 %tmp, 0
   br i1 %tmp1, label %.loopexit, label %.preheader
 
 .preheader:
   br label %bb2