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

[Scott Linder via Phabricator via llvm-commits]

scott.linder marked an inline comment as done.
scott.linder added a comment.

I will add some IR tests for at least those cases.

I don't understand what you mean about the const-32bit addrspace; can you explain more?



================
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) {
----------------
arsenm wrote:
> 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
I can work on this; would you prefer I do that in this patch?


================
Comment at: lib/Target/AMDGPU/SIInstrInfo.cpp:3490
+  unsigned VRsrc = Rsrc.getReg();
+  unsigned VRsrcUndef = getUndefRegState(Rsrc.isUndef());
+
----------------
arsenm wrote:
> This is a bool
I thought `isUndef` was a bool and `getUndefRegState` returned flags.


================
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);
----------------
arsenm wrote:
> Can you post a separate patch to fix the vector indexing loop not using this?
Yes, I was planning on doing that. Alternatively if I unify the two in this patch that will also handle it.


================
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
----------------
arsenm wrote:
> If you're not going to write out the variable checks for these, you're probably better off just using update_mir_test_checks
I will add the variable checks here.


Repository:
  rL LLVM

https://reviews.llvm.org/D50982