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 ↗

(On Diff #187050)

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

[cfang]

lib/Target/AMDGPU/SIInstrInfo.cpp

2218–2232 ↗

(On Diff #187050)

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 ↗

(On Diff #187272)

Typo oroginal

[arsenm]

lib/Target/AMDGPU/SILoadStoreOptimizer.cpp

264–265 ↗

(On Diff #187273)

This merged part of the other patch

[cfang]

lib/Target/AMDGPU/SILoadStoreOptimizer.cpp

264–265 ↗

(On Diff #187273)

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.

llvm/trunk/lib/Target/AMDGPU/SIInstrInfo.cpp

bool SIInstrInfo::areMemAccessesTriviallyDisjoint(MachineInstr &MIa,
 
   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);

llvm/trunk/lib/Target/AMDGPU/SILoadStoreOptimizer.cpp

if (Op.isReg()) {
                TargetRegisterInfo::isPhysicalRegister(Op.getReg()))
         PhysRegUses.insert(Op.getReg());
     }
   }
 }
 
 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()) {
for (MachineOperand &Use : MI.operands()) {
     }
   }
 
   return false;
 }
 
 static bool canMoveInstsAcrossMemOp(MachineInstr &MemOp,
                                     ArrayRef<MachineInstr *> InstsToMove,
-                                    const SIInstrInfo *TII, AliasAnalysis *AA) {
+                                    AliasAnalysis *AA) {
   assert(MemOp.mayLoadOrStore());
 
   for (MachineInstr *InstToMove : InstsToMove) {
     if (!InstToMove->mayLoadOrStore())
       continue;
-    if (!memAccessesCanBeReordered(MemOp, *InstToMove, TII, AA))
+    if (!memAccessesCanBeReordered(MemOp, *InstToMove, AA))
       return false;
   }
   return true;
 }
 
 bool SILoadStoreOptimizer::offsetsCanBeCombined(CombineInfo &CI) {
   // XXX - Would the same offset be OK? Is there any reason this would happen or
   // be useful?
if ((getInstClass(MBBI->getOpcode()) != InstClass) ||
 
       if (MBBI->hasUnmodeledSideEffects()) {
         // We can't re-order this instruction with respect to other memory
         // operations, so we fail both conditions mentioned above.
         return false;
       }
 
       if (MBBI->mayLoadOrStore() &&
-          (!memAccessesCanBeReordered(*CI.I, *MBBI, TII, AA) ||
-           !canMoveInstsAcrossMemOp(*MBBI, CI.InstsToMove, TII, AA))) {
+          (!memAccessesCanBeReordered(*CI.I, *MBBI, AA) ||
+           !canMoveInstsAcrossMemOp(*MBBI, CI.InstsToMove, AA))) {
         // We fail condition #1, but we may still be able to satisfy condition
         // #2.  Add this instruction to the move list and then we will check
         // if condition #2 holds once we have selected the matching instruction.
         CI.InstsToMove.push_back(&*MBBI);
         addDefsUsesToList(*MBBI, RegDefsToMove, PhysRegUsesToMove);
         continue;
       }
 
if (Match) {
         }
       }
 
       // Check both offsets fit in the reduced range.
       // We also need to go through the list of instructions that we plan to
       // move and make sure they are all safe to move down past the merged
       // instruction.
       if (widthsFit(*STM, CI) && offsetsCanBeCombined(CI))
-        if (canMoveInstsAcrossMemOp(*MBBI, CI.InstsToMove, TII, AA))
+        if (canMoveInstsAcrossMemOp(*MBBI, CI.InstsToMove, AA))
           return true;
     }
 
     // We've found a load/store that we couldn't merge for some reason.
     // We could potentially keep looking, but we'd need to make sure that
     // it was safe to move I and also all the instruction in InstsToMove
     // down past this instruction.
     // check if we can move I across MBBI and if we can move all I's users
-    if (!memAccessesCanBeReordered(*CI.I, *MBBI, TII, AA) ||
-        !canMoveInstsAcrossMemOp(*MBBI, CI.InstsToMove, TII, AA))
+    if (!memAccessesCanBeReordered(*CI.I, *MBBI, AA) ||
+        !canMoveInstsAcrossMemOp(*MBBI, CI.InstsToMove, AA))
       break;
   }
   return false;
 }
 
 unsigned SILoadStoreOptimizer::read2Opcode(unsigned EltSize) const {
   if (STM->ldsRequiresM0Init())
     return (EltSize == 4) ? AMDGPU::DS_READ2_B32 : AMDGPU::DS_READ2_B64;

llvm/trunk/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 original 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
+}