[PATCH] D39012: AMDGPU: Merge BUFFER_STORE_DWORD_OFFEN/OFFSET into x2, x4

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

One minor comment inline.

The volatile / hasOrderedMemoryRef issue is worrying, and I don't think the change should go in as-is. Besides, it's probably not as big a win as the other patches, especially due to the added VGPR spills. Do you have all these patches in a branch somewhere?



================
Comment at: lib/Target/AMDGPU/SILoadStoreOptimizer.cpp:768
+  if (CI.InstClass == BUFFER_STORE_OFFEN)
+      MIB = MIB.add(*TII->getNamedOperand(*CI.I, AMDGPU::OpName::vaddr));
+
----------------
Remove the assignment "MIB = MIB" here and below, it shouldn't be necessary. The return value is just a convenience to allow chains of .add(..).add(..) etc.


================
Comment at: test/CodeGen/AMDGPU/fmul-2-combine-multi-use.ll:42-51
+; GCN-DAG: v_add_f32_e64 v[[MUL2:[0-9]+]], [[X:s[0-9]+]], s{{[0-9]+}}
+; GCN-DAG: v_mac_f32_e64 v[[MAD:[0-9]+]], [[X]], 2.0
+; GCN-DAG: buffer_store_dwordx2 v{{\[}}[[MUL2]]:[[MAD]]]
 ; GCN: s_endpgm
 define amdgpu_kernel void @multiple_use_fadd_fmac_f32(float addrspace(1)* %out, float %x, float %y) #0 {
   %out.gep.1 = getelementptr float, float addrspace(1)* %out, i32 1
   %mul2 = fmul fast float %x, 2.0
----------------
In this case, the stores are explicitly marked as volatile in the input IR. Merging those seems like a very bad idea.


================
Comment at: test/CodeGen/AMDGPU/fmul-2-combine-multi-use.ll:56-66
+; GCN-DAG: v_add_f32_e64 v[[MUL2:[0-9]+]], |[[X:s[0-9]+]]|, |s{{[0-9]+}}|
+; GCN-DAG: v_mad_f32 v[[MAD:[0-9]+]], |[[X]]|, 2.0, v{{[0-9]+}}
+; GCN-DAG: buffer_store_dwordx2 v{{\[}}[[MUL2]]:[[MAD]]]
 ; GCN: s_endpgm
 define amdgpu_kernel void @multiple_use_fadd_fmad_f32(float addrspace(1)* %out, float %x, float %y) #0 {
   %out.gep.1 = getelementptr float, float addrspace(1)* %out, i32 1
   %x.abs = call float @llvm.fabs.f32(float %x)
----------------
Same here.


https://reviews.llvm.org/D39012