[AMDGPU] stop image_store being moved illegally

[tpr]

Summary

A recent change
321556: AMDGPU: Remove mayLoad/hasSideEffects from MIMG stores
can allow the machine instruction scheduler to move an image store past
an image load using the same descriptor.

As a temporary workaround until we figure out what is going wrong with
alias analysis, this commit makes all image load and store instructions
volatile.

[tstellar]

I think changing AMDGPUImagePseudoSourceValue::isAliased() to return true may fix this.

[tstellar]

In D41969#974049, @tstellar wrote:

I think changing AMDGPUImagePseudoSourceValue::isAliased() to return true may fix this.

This is the most conservative way to fix it. I think the better way to fix this would be to change AMDGPUImagePseudoSourceValue::mayAlias() to true. You could improve this even further by making sure that image instructions that reference the same resource have the same PseudoSourceValue. ScheduleDAGInstrs::buildSchedGraph() is where all the dependencies are calculated.

[tpr]

V2: The more conservtive of Tom's suggested fixes.

[tpr]

Thanks Tom. Just setting mayAlias() didn't fix it. So I have gone with setting isAliased() as well. We should revisit this, but maybe it's not worth it until we get round to doing fat pointers.

[tpr]

V3: Reverted test change done on 321556.

[arsenm]

In D41969#974088, @tstellar wrote:

In D41969#974049, @tstellar wrote:

I think changing AMDGPUImagePseudoSourceValue::isAliased() to return true may fix this.

This is the most conservative way to fix it. I think the better way to fix this would be to change AMDGPUImagePseudoSourceValue::mayAlias() to true. You could improve this even further by making sure that image instructions that reference the same resource have the same PseudoSourceValue. ScheduleDAGInstrs::buildSchedGraph() is where all the dependencies are calculated.

That images have the same resource argument do have the same PSV now. I think the issue may be the non-constant offsets in the intrinsics aren't represented anywhere

[tstellar]

In D41969#974642, @arsenm wrote:

In D41969#974088, @tstellar wrote:

In D41969#974049, @tstellar wrote:

I think changing AMDGPUImagePseudoSourceValue::isAliased() to return true may fix this.

This is the most conservative way to fix it. I think the better way to fix this would be to change AMDGPUImagePseudoSourceValue::mayAlias() to true. You could improve this even further by making sure that image instructions that reference the same resource have the same PseudoSourceValue. ScheduleDAGInstrs::buildSchedGraph() is where all the dependencies are calculated.

That images have the same resource argument do have the same PSV now. I think the issue may be the non-constant offsets in the intrinsics aren't represented anywhere

Ok, it looks like maybe just setting the MachineMemOperand size to something non-zero may work for now.

[tstellar]

As is, this patch is fine. Other improvements can be done as follow on work.

llvm/trunk/lib/Target/AMDGPU/SIMachineFunctionInfo.h

public:
 
   bool isConstant(const MachineFrameInfo *) const override {
     // This should probably be true for most images, but we will start by being
     // conservative.
     return false;
   }
 
   bool isAliased(const MachineFrameInfo *) const override {
-    // FIXME: If we ever change image intrinsics to accept fat pointers, then
-    // this could be true for some cases.
-    return false;
+    return true;
   }
 
   bool mayAlias(const MachineFrameInfo *) const override {
-    // FIXME: If we ever change image intrinsics to accept fat pointers, then
-    // this could be true for some cases.
-    return false;
+    return true;
   }
 };
 
 class AMDGPUBufferPseudoSourceValue : public PseudoSourceValue {
 public:
   explicit AMDGPUBufferPseudoSourceValue(const TargetInstrInfo &TII) :
     PseudoSourceValue(PseudoSourceValue::TargetCustom, TII) { }
 

llvm/trunk/test/CodeGen/AMDGPU/image-schedule.ll

+; RUN: llc -march=amdgcn -mcpu=tonga -verify-machineinstrs < %s | FileCheck -check-prefixes=GCN %s
+
+target datalayout = "e-p:32:32-p1:64:64-p2:64:64-p3:32:32-p4:64:64-p5:32:32-i64:64-v16:16-v24:32-v32:32-v48:64-v96:128-v192:256-v256:256-v512:512-v1024:1024-v2048:2048-n32:64"
+target triple = "amdgcn--amdpal"
+
+; The first image store and the second image load use the same descriptor and
+; the same coordinate. Check that they do not get swapped by the machine
+; instruction scheduler.
+
+; GCN-LABEL: {{^}}_amdgpu_cs_main:
+; GCN: image_load
+; GCN: image_store
+; GCN: image_load
+; GCN: image_store
+
+define dllexport amdgpu_cs void @_amdgpu_cs_main(i32 inreg %arg, i32 inreg %arg1, i32 inreg %arg2, <3 x i32> inreg %arg3, i32 inreg %arg4, <3 x i32> %arg5) local_unnamed_addr #0 {
+.entry:
+  %tmp = call i64 @llvm.amdgcn.s.getpc() #1
+  %tmp6 = bitcast i64 %tmp to <2 x i32>
+  %.0.vec.insert = insertelement <2 x i32> undef, i32 %arg2, i32 0
+  %.4.vec.insert = shufflevector <2 x i32> %.0.vec.insert, <2 x i32> %tmp6, <2 x i32> <i32 0, i32 3>
+  %tmp7 = bitcast <2 x i32> %.4.vec.insert to i64
+  %tmp8 = inttoptr i64 %tmp7 to [4294967295 x i8] addrspace(2)*
+  %tmp9 = add <3 x i32> %arg3, %arg5
+  %tmp10 = getelementptr [4294967295 x i8], [4294967295 x i8] addrspace(2)* %tmp8, i64 0, i64 32
+  %tmp11 = bitcast i8 addrspace(2)* %tmp10 to <8 x i32> addrspace(2)*, !amdgpu.uniform !0
+  %tmp12 = load <8 x i32>, <8 x i32> addrspace(2)* %tmp11, align 16
+  %tmp13 = shufflevector <3 x i32> %tmp9, <3 x i32> undef, <2 x i32> <i32 0, i32 1>
+  %tmp14 = call <4 x float> @llvm.amdgcn.image.load.v4f32.v2i32.v8i32(<2 x i32> %tmp13, <8 x i32> %tmp12, i32 15, i1 false, i1 false, i1 false, i1 false) #0
+  %tmp15 = inttoptr i64 %tmp7 to <8 x i32> addrspace(2)*
+  %tmp16 = load <8 x i32>, <8 x i32> addrspace(2)* %tmp15, align 16
+  call void @llvm.amdgcn.image.store.v4f32.v2i32.v8i32(<4 x float> %tmp14, <2 x i32> %tmp13, <8 x i32> %tmp16, i32 15, i1 false, i1 false, i1 false, i1 false) #0
+  %tmp17 = load <8 x i32>, <8 x i32> addrspace(2)* %tmp15, align 16
+  %tmp18 = call <4 x float> @llvm.amdgcn.image.load.v4f32.v2i32.v8i32(<2 x i32> %tmp13, <8 x i32> %tmp17, i32 15, i1 false, i1 false, i1 false, i1 false) #0
+  %tmp19 = getelementptr [4294967295 x i8], [4294967295 x i8] addrspace(2)* %tmp8, i64 0, i64 64
+  %tmp20 = bitcast i8 addrspace(2)* %tmp19 to <8 x i32> addrspace(2)*, !amdgpu.uniform !0
+  %tmp21 = load <8 x i32>, <8 x i32> addrspace(2)* %tmp20, align 16
+  call void @llvm.amdgcn.image.store.v4f32.v2i32.v8i32(<4 x float> %tmp18, <2 x i32> %tmp13, <8 x i32> %tmp21, i32 15, i1 false, i1 false, i1 false, i1 false) #0
+  ret void
+}
+
+; Function Attrs: nounwind readnone speculatable
+declare i64 @llvm.amdgcn.s.getpc() #1
+
+; Function Attrs: nounwind readonly
+declare <4 x float> @llvm.amdgcn.image.load.v4f32.v2i32.v8i32(<2 x i32>, <8 x i32>, i32, i1, i1, i1, i1) #2
+
+; Function Attrs: nounwind writeonly
+declare void @llvm.amdgcn.image.store.v4f32.v2i32.v8i32(<4 x float>, <2 x i32>, <8 x i32>, i32, i1, i1, i1, i1) #3
+
+attributes #0 = { nounwind }
+attributes #1 = { nounwind readnone speculatable }
+attributes #2 = { nounwind readonly }
+attributes #3 = { nounwind writeonly }
+
+!0 = !{}

llvm/trunk/test/CodeGen/AMDGPU/llvm.amdgcn.image.ll

main_body:
   %r2 = extractelement <4 x float> %r, i32 2
   %r3 = extractelement <4 x float> %r, i32 3
   call void @llvm.amdgcn.exp.f32(i32 0, i32 15, float %r0, float %r1, float %r2, float %r3, i1 true, i1 true) #0
   ret void
 }
 
 ; Ideally, the register allocator would avoid the wait here
 ;
-; XXX - Is this really allowed? Are the resource descriptors allowed to alias?
 ; GCN-LABEL: {{^}}image_store_wait:
-; GCN: image_load v[5:8], v4, s[8:15] dmask:0xf unorm
 ; GCN: image_store v[0:3], v4, s[0:7] dmask:0xf unorm
-; GCN: s_waitcnt vmcnt(1)
-; GCN: image_store v[5:8], v4, s[16:23] dmask:0xf unorm
-; GCN-NEXT: s_endpgm
+; GCN: s_waitcnt expcnt(0)
+; GCN: image_load v[0:3], v4, s[8:15] dmask:0xf unorm
+; GCN: s_waitcnt vmcnt(0)
+; GCN: image_store v[0:3], v4, s[16:23] dmask:0xf unorm
 define amdgpu_ps void @image_store_wait(<8 x i32> inreg %arg, <8 x i32> inreg %arg1, <8 x i32> inreg %arg2, <4 x float> %arg3, i32 %arg4) #0 {
 main_body:
   call void @llvm.amdgcn.image.store.v4f32.i32.v8i32(<4 x float> %arg3, i32 %arg4, <8 x i32> %arg, i32 15, i1 false, i1 false, i1 false, i1 false)
   %data = call <4 x float> @llvm.amdgcn.image.load.v4f32.i32.v8i32(i32 %arg4, <8 x i32> %arg1, i32 15, i1 false, i1 false, i1 false, i1 false)
   call void @llvm.amdgcn.image.store.v4f32.i32.v8i32(<4 x float> %data, i32 %arg4, <8 x i32> %arg2, i32 15, i1 false, i1 false, i1 false, i1 false)
   ret void
 }
 
-; The same image resource is used so reordering is not OK.
-; GCN-LABEL: {{^}}image_store_wait_same_resource:
-; GCN: image_store v[0:3], v4, s[0:7] dmask:0xf unorm
-; GCN: s_waitcnt expcnt(0)
-; GCN: image_load v[0:3], v4, s[0:7] dmask:0xf unorm
-; GCN: s_waitcnt vmcnt(0)
-; GCN: image_store v[0:3], v4, s[0:7] dmask:0xf unorm
-define amdgpu_ps void @image_store_wait_same_resource(<8 x i32> inreg %rsrc, <4 x float> %arg3, i32 %arg4) #0 {
-main_body:
-  call void @llvm.amdgcn.image.store.v4f32.i32.v8i32(<4 x float> %arg3, i32 %arg4, <8 x i32> %rsrc, i32 15, i1 false, i1 false, i1 false, i1 false)
-  %data = call <4 x float> @llvm.amdgcn.image.load.v4f32.i32.v8i32(i32 %arg4, <8 x i32> %rsrc, i32 15, i1 false, i1 false, i1 false, i1 false)
-  call void @llvm.amdgcn.image.store.v4f32.i32.v8i32(<4 x float> %data, i32 %arg4, <8 x i32> %rsrc, i32 15, i1 false, i1 false, i1 false, i1 false)
-  ret void
-}
-
 ; SI won't merge ds memory operations, because of the signed offset bug, so
 ; we only have check lines for VI.
 ; VI-LABEL: image_load_mmo
 ; VI: v_mov_b32_e32 [[ZERO:v[0-9]+]], 0
 ; VI: ds_write2_b32 v{{[0-9]+}}, [[ZERO]], [[ZERO]] offset1:4
 define amdgpu_ps void @image_load_mmo(float addrspace(3)* %lds, <2 x i32> %c, <8 x i32> inreg %rsrc) #0 {
 bb:
   store float 0.000000e+00, float addrspace(3)* %lds