[PATCH] D25428: AMDGPU add support for spilling to a user sgpr pointed buffers

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

This is missing the part touching the argument lowering. When the input sgpr0/1 is going to be used it needs to be marked in the initial argument lowering in LowerFormalArguments, or else the other argument usage might end up thinking it can use these



================
Comment at: lib/Target/AMDGPU/SIFrameLowering.cpp:316
 
-    BuildMI(MBB, I, DL, SMovB32, Rsrc1)
-      .addExternalSymbol("SCRATCH_RSRC_DWORD1")
-      .addReg(ScratchRsrcReg, RegState::ImplicitDefine);
+    if (MFI->hasSpillUserPtr()) {
+      unsigned Rsrc01 = TRI->getSubReg(ScratchRsrcReg, AMDGPU::sub0_sub1);
----------------
I think this needs a better name, that definitely doesn't involve the word spill. We currently mix the terms scratch and private memory. How about hasPrivateMemoryPointerInput()? 


================
Comment at: lib/Target/AMDGPU/SIFrameLowering.cpp:326
+        .addReg(ScratchRsrcReg, RegState::ImplicitDefine);
+    } else if (MFI->hasSpillUserPtrLoad()) {
+      unsigned Rsrc01 = TRI->getSubReg(ScratchRsrcReg, AMDGPU::sub0_sub1);
----------------
hasIndirectPrivaetMemoryPointerInput? I assume you are putting other things in this buffer besides just this one pointer, so maybe the name should be whatever you want to call that buffer.


================
Comment at: lib/Target/AMDGPU/SIFrameLowering.cpp:333-335
+      PointerType *PtrTy = PointerType::get(Type::getInt64Ty(MF.getFunction()->getContext()), AMDGPUAS::CONSTANT_ADDRESS);
+      MachinePointerInfo PtrInfo(UndefValue::get(PtrTy));
+      auto MMO = MF.getMachineMemOperand(PtrInfo, MachineMemOperand::MOLoad | MachineMemOperand::MOInvariant |
----------------
These go over 80 lines


================
Comment at: lib/Target/AMDGPU/SIFrameLowering.cpp:338-340
+        .addReg(Sgpr01)
+        .addImm(0)
+        .addImm(0)
----------------
Usually we put a comment with the name of the operand after each one for the more complicated instructions, e.g. // offset


================
Comment at: lib/Target/AMDGPU/SIMachineFunctionInfo.cpp:132-135
+    if (F->hasFnAttribute("amdgpu-spill-bufsgpr01"))
+      SpillUserPtr = true;
+    if (F->hasFnAttribute("amdgpu-spill-bufsgpr01-load"))
+      SpillUserPtrLoad = true;
----------------
I  don't really  like using the attributes this way naming specific registers. This needs to always be available, so I don't see why you need to explicitly enable this particularly in the indirect case.


https://reviews.llvm.org/D25428