AMDGPU: Fix memory dependence analysis by considering the offset.

[cfang]

Summary

In SIInstrInfo::areMemAccessesTriviallyDisjoint, the current implementation ignored the offset when creates MemoryLocation
for alias analysis, and thus may report wrong result when offset is not zero. This patch fixes the issue.

[arsenm]

lib/Target/AMDGPU/SIInstrInfo.cpp

2218–2232

Why can't you use MI.mayAlias and delete this?

[cfang]

lib/Target/AMDGPU/SIInstrInfo.cpp

2218–2232

This intends to fix a bug in an application. We can use MI.mayAlias and delete areMemAccessesTriviallyDisjoint.
However, we need to make sure areMemAccessesTriviallyDisjoint is indeed useless because things developed there
are not for on reason. We need extensive tests.
So I propose to add this patch first to fix the bug. Then we can spend time to investigate the way to safely remove the stuff in
areMemAccessesTriviallyDisjoint and replace it with MI.mayAlias.

[cfang]

Use MachineInstr::mayAlias to replace areMemAccessesTriviallyDisjoint in LoadStoreOptimizer pass.

[cfang]

Remove the unused Target Instruction Info argument in a couple functions.

[arsenm]

LGTM

test/CodeGen/AMDGPU/ds-combine-with-dependence.ll

69

Typo oroginal

[arsenm]

lib/Target/AMDGPU/SILoadStoreOptimizer.cpp

264–265

This merged part of the other patch

[cfang]

lib/Target/AMDGPU/SILoadStoreOptimizer.cpp

264–265

What do you mean here? I think this is the staring point of the current patch. We are going to replace
TII->areMemAccessesTriviallyDisjoint with MI.mayAlias to fix the bug.

lib/Target/AMDGPU/SIInstrInfo.cpp

 
   if (MIa.hasUnmodeledSideEffects() || MIb.hasUnmodeledSideEffects())
     return false;
 
   // XXX - Can we relax this between address spaces?
   if (MIa.hasOrderedMemoryRef() || MIb.hasOrderedMemoryRef())
     return false;
 
-  if (AA && MIa.hasOneMemOperand() && MIb.hasOneMemOperand()) {
-    const MachineMemOperand *MMOa = *MIa.memoperands_begin();
-    const MachineMemOperand *MMOb = *MIb.memoperands_begin();
-    if (MMOa->getValue() && MMOb->getValue()) {
-      MemoryLocation LocA(MMOa->getValue(), MMOa->getSize(), MMOa->getAAInfo());
-      MemoryLocation LocB(MMOb->getValue(), MMOb->getSize(), MMOb->getAAInfo());
-      if (!AA->alias(LocA, LocB))
-        return true;
-    }
-  }
-
   // TODO: Should we check the address space from the MachineMemOperand? That
   // would allow us to distinguish objects we know don't alias based on the
   // underlying address space, even if it was lowered to a different one,
   // e.g. private accesses lowered to use MUBUF instructions on a scratch
   // buffer.
   if (isDS(MIa)) {
     if (isDS(MIb))
       return checkInstOffsetsDoNotOverlap(MIa, MIb);
 
     return !isFLAT(MIb) || isSegmentSpecificFLAT(MIb);
   }
 
   if (isMUBUF(MIa) || isMTBUF(MIa)) {
     if (isMUBUF(MIb) || isMTBUF(MIb))
       return checkInstOffsetsDoNotOverlap(MIa, MIb);
 
     return !isFLAT(MIb) && !isSMRD(MIb);
   }
 
   if (isSMRD(MIa)) {
     if (isSMRD(MIb))
       return checkInstOffsetsDoNotOverlap(MIa, MIb);
 
     return !isFLAT(MIb) && !isMUBUF(MIa) && !isMTBUF(MIa);
   }
 
   if (isFLAT(MIa)) {
     if (isFLAT(MIb))
       return checkInstOffsetsDoNotOverlap(MIa, MIb);
 

lib/Target/AMDGPU/SILoadStoreOptimizer.cpp

 
 static bool memAccessesCanBeReordered(MachineBasicBlock::iterator A,
                                       MachineBasicBlock::iterator B,
                                       const SIInstrInfo *TII,
                                       AliasAnalysis *AA) {
   // RAW or WAR - cannot reorder
   // WAW - cannot reorder
   // RAR - safe to reorder
-  return !(A->mayStore() || B->mayStore()) ||
-         TII->areMemAccessesTriviallyDisjoint(*A, *B, AA);
+  return !(A->mayStore() || B->mayStore()) || !A->mayAlias(AA, *B, true);
 }
 
 // Add MI and its defs to the lists if MI reads one of the defs that are
 // already in the list. Returns true in that case.
 static bool addToListsIfDependent(MachineInstr &MI, DenseSet<unsigned> &RegDefs,
                                   DenseSet<unsigned> &PhysRegUses,
                                   SmallVectorImpl<MachineInstr *> &Insts) {
   for (MachineOperand &Use : MI.operands()) {

test/CodeGen/AMDGPU/ds-combine-with-dependence.ll

+; RUN: llc -march=amdgcn -mcpu=gfx900 -verify-machineinstrs < %s | FileCheck -check-prefixes=GCN %s
+
+
+; There is no dependence between the store and the two loads. So we can combine the loads
+; and the combined load is at the original place of the second load.
+
+; GCN-LABEL: {{^}}ds_combine_nodep
+
+; GCN:      ds_write2_b32 v{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}} offset0:26 offset1:27
+; GCN-NEXT: ds_read2_b32 v{{\[[0-9]+:[0-9]+\]}}, v{{[0-9]+}} offset0:7 offset1:8
+define amdgpu_kernel void @ds_combine_nodep(float addrspace(1)* %out, float addrspace(3)* %inptr) {
+
+  %base = bitcast float addrspace(3)* %inptr to i8 addrspace(3)*
+  %addr0 = getelementptr i8, i8 addrspace(3)* %base, i32 24
+  %tmp0 = bitcast i8 addrspace(3)* %addr0 to float addrspace(3)*
+  %vaddr0 = bitcast float addrspace(3)* %tmp0 to <3 x float> addrspace(3)*
+  %load0 = load <3 x float>, <3 x float> addrspace(3)* %vaddr0, align 4
+  %v0 = extractelement <3 x float> %load0, i32 2
+
+  %tmp1 = insertelement <2 x float> undef, float 1.0, i32 0
+  %data = insertelement <2 x float> %tmp1, float 2.0, i32 1
+
+  %tmp2 = getelementptr float, float addrspace(3)* %inptr, i32 26
+  %vaddrs = bitcast float addrspace(3)* %tmp2 to <2 x float> addrspace(3)*
+  store <2 x float> %data, <2 x float> addrspace(3)* %vaddrs, align 4
+
+  %vaddr1 = getelementptr float, float addrspace(3)* %inptr, i32 7
+  %v1 = load float, float addrspace(3)* %vaddr1, align 4
+
+  %sum = fadd float %v0, %v1
+  store float %sum, float addrspace(1)* %out, align 4
+  ret void
+}
+
+
+; The store depends on the first load, so we could not move the first load down to combine with
+; the second load directly. However, we can move the store after the combined load.
+
+; GCN-LABEL: {{^}}ds_combine_WAR
+
+; GCN:      ds_read2_b32 v{{\[[0-9]+:[0-9]+\]}}, v{{[0-9]+}} offset0:7 offset1:27
+; GCN-NEXT: ds_write2_b32 v{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}} offset0:26 offset1:27
+define amdgpu_kernel void @ds_combine_WAR(float addrspace(1)* %out, float addrspace(3)* %inptr) {
+
+  %base = bitcast float addrspace(3)* %inptr to i8 addrspace(3)*
+  %addr0 = getelementptr i8, i8 addrspace(3)* %base, i32 100
+  %tmp0 = bitcast i8 addrspace(3)* %addr0 to float addrspace(3)*
+  %vaddr0 = bitcast float addrspace(3)* %tmp0 to <3 x float> addrspace(3)*
+  %load0 = load <3 x float>, <3 x float> addrspace(3)* %vaddr0, align 4
+  %v0 = extractelement <3 x float> %load0, i32 2
+
+  %tmp1 = insertelement <2 x float> undef, float 1.0, i32 0
+  %data = insertelement <2 x float> %tmp1, float 2.0, i32 1
+
+  %tmp2 = getelementptr float, float addrspace(3)* %inptr, i32 26
+  %vaddrs = bitcast float addrspace(3)* %tmp2 to <2 x float> addrspace(3)*
+  store <2 x float> %data, <2 x float> addrspace(3)* %vaddrs, align 4
+
+  %vaddr1 = getelementptr float, float addrspace(3)* %inptr, i32 7
+  %v1 = load float, float addrspace(3)* %vaddr1, align 4
+
+  %sum = fadd float %v0, %v1
+  store float %sum, float addrspace(1)* %out, align 4
+  ret void
+}
+
+
+; The second load depends on the store. We can combine the two loads, and the combined load is
+; at the oroginal place of the second load.
+
+; GCN-LABEL: {{^}}ds_combine_RAW
+
+; GCN:      ds_write2_b32 v{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}} offset0:26 offset1:27
+; GCN-NEXT: ds_read2_b32 v{{\[[0-9]+:[0-9]+\]}}, v{{[0-9]+}} offset0:8 offset1:26
+define amdgpu_kernel void @ds_combine_RAW(float addrspace(1)* %out, float addrspace(3)* %inptr) {
+
+  %base = bitcast float addrspace(3)* %inptr to i8 addrspace(3)*
+  %addr0 = getelementptr i8, i8 addrspace(3)* %base, i32 24
+  %tmp0 = bitcast i8 addrspace(3)* %addr0 to float addrspace(3)*
+  %vaddr0 = bitcast float addrspace(3)* %tmp0 to <3 x float> addrspace(3)*
+  %load0 = load <3 x float>, <3 x float> addrspace(3)* %vaddr0, align 4
+  %v0 = extractelement <3 x float> %load0, i32 2
+
+  %tmp1 = insertelement <2 x float> undef, float 1.0, i32 0
+  %data = insertelement <2 x float> %tmp1, float 2.0, i32 1
+
+  %tmp2 = getelementptr float, float addrspace(3)* %inptr, i32 26
+  %vaddrs = bitcast float addrspace(3)* %tmp2 to <2 x float> addrspace(3)*
+  store <2 x float> %data, <2 x float> addrspace(3)* %vaddrs, align 4
+
+  %vaddr1 = getelementptr float, float addrspace(3)* %inptr, i32 26
+  %v1 = load float, float addrspace(3)* %vaddr1, align 4
+
+  %sum = fadd float %v0, %v1
+  store float %sum, float addrspace(1)* %out, align 4
+  ret void
+}
+
+
+; The store depends on the first load, also the second load depends on the store.
+; So we can not combine the two loads.
+
+; GCN-LABEL: {{^}}ds_combine_WAR_RAW
+
+; GCN:      ds_read_b32 v{{[0-9]+}}, v{{[0-9]+}} offset:108
+; GCN-NEXT: ds_write2_b32 v{{[0-9]+}}, v{{[0-9]+}}, v{{[0-9]+}} offset0:26 offset1:27
+; GCN-NEXT: ds_read_b32 v{{[0-9]+}}, v{{[0-9]+}} offset:104
+define amdgpu_kernel void @ds_combine_WAR_RAW(float addrspace(1)* %out, float addrspace(3)* %inptr) {
+
+  %base = bitcast float addrspace(3)* %inptr to i8 addrspace(3)*
+  %addr0 = getelementptr i8, i8 addrspace(3)* %base, i32 100
+  %tmp0 = bitcast i8 addrspace(3)* %addr0 to float addrspace(3)*
+  %vaddr0 = bitcast float addrspace(3)* %tmp0 to <3 x float> addrspace(3)*
+  %load0 = load <3 x float>, <3 x float> addrspace(3)* %vaddr0, align 4
+  %v0 = extractelement <3 x float> %load0, i32 2
+
+  %tmp1 = insertelement <2 x float> undef, float 1.0, i32 0
+  %data = insertelement <2 x float> %tmp1, float 2.0, i32 1
+
+  %tmp2 = getelementptr float, float addrspace(3)* %inptr, i32 26
+  %vaddrs = bitcast float addrspace(3)* %tmp2 to <2 x float> addrspace(3)*
+  store <2 x float> %data, <2 x float> addrspace(3)* %vaddrs, align 4
+
+  %vaddr1 = getelementptr float, float addrspace(3)* %inptr, i32 26
+  %v1 = load float, float addrspace(3)* %vaddr1, align 4
+
+  %sum = fadd float %v0, %v1
+  store float %sum, float addrspace(1)* %out, align 4
+  ret void
+}