[PATCH] D50982: [AMDGPU] Legalize VGPR Rsrc operands for MUBUF instructions

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In this case I think we should have some pure-IR tests since we'll need to worry about this in GlobalISel as well.

You are missing some tests involving control flow that are likely to be broken. Some cases I would worry about:

- Expansion outside of the entry block, with values live across block boundaries
- Expansion of two adjacent operations that introduce control flow
- Expansion in multiple blocks in the function

I think there were a few other problematic cases I ran into for the vector indexing waterfall



================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:3481-3486
+static void emitLoadSRsrcFromVGPRLoop(const SIInstrInfo &TII,
+                                      MachineRegisterInfo &MRI,
+                                      MachineBasicBlock &OrigBB,
+                                      MachineBasicBlock &LoopBB,
+                                      const DebugLoc &DL,
+                                      MachineOperand &Rsrc) {
----------------
I was hoping for this to a be a more general function that could be re-used for other operations in the future, and probably sharable with the vector indexing code.

I was thinking about something like a list of operands that need to be SGPRs, and iterating over all of the subregisters rather than assuming the SGPR128 +  sometimes SGPR32 the buffer operations use


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:3490
+  unsigned VRsrc = Rsrc.getReg();
+  unsigned VRsrcUndef = getUndefRegState(Rsrc.isUndef());
+
----------------
This is a bool


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:3546-3548
+  BuildMI(LoopBB, I, DL, TII.get(AMDGPU::S_XOR_B64_term), AMDGPU::EXEC)
+    .addReg(AMDGPU::EXEC)
+    .addReg(SaveExec);
----------------
Can you post a separate patch to fix the vector indexing loop not using this?


================
Comment at: test/CodeGen/AMDGPU/mubuf-legalize-operands.mir:1
+# RUN: llc -march=amdgcn -mcpu=gfx700 --run-pass=si-fix-sgpr-copies -o - %s | FileCheck %s --check-prefixes=COMMON,ADDR64
+# RUN: llc -march=amdgcn -mcpu=gfx900 --run-pass=si-fix-sgpr-copies -o - %s | FileCheck %s --check-prefixes=COMMON,NO-ADDR64
----------------
I think this needs a -verify-machineinstrs


================
Comment at: test/CodeGen/AMDGPU/mubuf-legalize-operands.mir:16-17
+# COMMON-NEXT: successors: %bb.1({{.*}}), %bb.2({{.*}})
+# COMMON: %14:sgpr_32 = V_READFIRSTLANE_B32 %8.sub0, implicit $exec
+# COMMON: %15:sgpr_32 = V_READFIRSTLANE_B32 %8.sub1, implicit $exec
+# COMMON: %16:sgpr_32 = V_READFIRSTLANE_B32 %8.sub2, implicit $exec
----------------
If you're not going to write out the variable checks for these, you're probably better off just using update_mir_test_checks


Repository:
  rL LLVM

https://reviews.llvm.org/D50982