AMDGPU: Fix SILoadStoreOptimizer when writes cannot be merged due register dependencies

[nhaehnle]

Summary

When finding a match for a merge and collecting the instructions that must
be moved, keep in mind that the instruction we merge might actually use one
of the defs that are being moved.

Fixes piglit spec/arb_enhanced_layouts/execution/component-layout/vs-tcs-load-output[-indirect].

The fact that the ds_read in the test case is not eliminated suggests that
there might be another problem related to alias analysis, but that's a
separate problem: this pass should still work correctly even when earlier
optimization passes missed something or were disabled.

[• tstellarAMD]

lib/Target/AMDGPU/SILoadStoreOptimizer.cpp

259

What does f(w) mean in this comment? Also, before this patch was this pass merging these two writes?

[nhaehnle]

lib/Target/AMDGPU/SILoadStoreOptimizer.cpp

259

It's supposed to mean some function of w :)

Before this patch, the writes were merged and compilation failed later because "use not dominated by defs".

[• tstellarAMD]

LGTM.

[arsenm]

Should this be using MachineInstr::isSafeToMove instead of this custom logic?

[arsenm]

In D25829#577494, @arsenm wrote:

Should this be using MachineInstr::isSafeToMove instead of this custom logic?

After looking at the implementation this isn't right. The comment for it seems misleading to me since it doesn't take an iterator for where you're moving it, and only checks a few things on the single instruction

lib/Target/AMDGPU/SILoadStoreOptimizer.cpp

 
 static void addDefsToList(const MachineInstr &MI,
                           SmallVectorImpl<const MachineOperand *> &Defs) {
   for (const MachineOperand &Def : MI.defs()) {
     Defs.push_back(&Def);
   }
 }
 
+// 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,
+                      SmallVectorImpl<const MachineOperand *> &Defs,
+                      SmallVectorImpl<MachineInstr*> &Insts) {
+  for (const MachineOperand *Def : Defs) {
+    bool ReadDef = MI.readsVirtualRegister(Def->getReg());
+    // If ReadDef is true, then there is a use of Def between I
+    // and the instruction that I will potentially be merged with. We
+    // will need to move this instruction after the merged instructions.
+    if (ReadDef) {
+      Insts.push_back(&MI);
+      addDefsToList(MI, Defs);
+      return true;
+    }
+  }
+
+  return false;
+}
+
 static bool
 canMoveInstsAcrossMemOp(MachineInstr &MemOp,
                         ArrayRef<MachineInstr*> InstsToMove,
                         const SIInstrInfo *TII,
                         AliasAnalysis *AA) {
 
   assert(MemOp.mayLoadOrStore());
 
if (MBBI->getOpcode() != I->getOpcode()) {
         InstsToMove.push_back(&*MBBI);
         addDefsToList(*MBBI, DefsToMove);
         continue;
       }
 
       // When we match I with another DS instruction we will be moving I down
       // to the location of the matched instruction any uses of I will need to
       // be moved down as well.
-      for (const MachineOperand *Def : DefsToMove) {
-        bool ReadDef = MBBI->readsVirtualRegister(Def->getReg());
-        // If ReadDef is true, then there is a use of Def between I
-        // and the instruction that I will potentially be merged with. We
-        // will need to move this instruction after the merged instructions.
-        if (ReadDef) {
-          InstsToMove.push_back(&*MBBI);
-          addDefsToList(*MBBI, DefsToMove);
-          break;
-        }
-      }
+      addToListsIfDependent(*MBBI, DefsToMove, InstsToMove);
       continue;
     }
 
     // Don't merge volatiles.
     if (MBBI->hasOrderedMemoryRef())
       return E;
 
+    // Handle a case like
+    //   DS_WRITE_B32 addr, v, idx0
+    //   w = DS_READ_B32 addr, idx0
+    //   DS_WRITE_B32 addr, f(w), idx1
+    // where the DS_READ_B32 ends up in InstsToMove and therefore prevents
+    // merging of the two writes.
+    if (addToListsIfDependent(*MBBI, DefsToMove, InstsToMove))
+      continue;
+
     int AddrIdx = AMDGPU::getNamedOperandIdx(I->getOpcode(), AMDGPU::OpName::addr);
     const MachineOperand &AddrReg0 = I->getOperand(AddrIdx);
     const MachineOperand &AddrReg1 = MBBI->getOperand(AddrIdx);
 
     // Check same base pointer. Be careful of subregisters, which can occur with
     // vectors of pointers.
     if (AddrReg0.getReg() == AddrReg1.getReg() &&
         AddrReg0.getSubReg() == AddrReg1.getSubReg()) {

test/CodeGen/AMDGPU/merge-store-usedef.ll

+; RUN: llc -march=amdgcn -mcpu=verde -verify-machineinstrs < %s | FileCheck %s
+; RUN: llc -march=amdgcn -mcpu=tonga -verify-machineinstrs < %s | FileCheck %s
+
+; CHECK-LABEL: {{^}}test1:
+; CHECK: ds_write_b32
+; CHECK: ds_read_b32
+; CHECK: ds_write_b32
+define amdgpu_vs void @test1(i32 %v) #0 {
+  %p0 = getelementptr i32, i32 addrspace(3)* null, i32 0
+  %p1 = getelementptr i32, i32 addrspace(3)* null, i32 1
+
+  store i32 %v, i32 addrspace(3)* %p0
+
+  call void @llvm.SI.tbuffer.store.i32(<16 x i8> undef, i32 %v, i32 1, i32 undef, i32 undef, i32 0, i32 4, i32 4, i32 1, i32 0, i32 1, i32 1, i32 0)
+
+  %w = load i32, i32 addrspace(3)* %p0
+  store i32 %w, i32 addrspace(3)* %p1
+  ret void
+}
+
+declare void @llvm.SI.tbuffer.store.i32(<16 x i8>, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32, i32) #0
+
+attributes #0 = { nounwind }