[PATCH] D99269: [AMDGPU] Unify spill code

[Scott Linder via Phabricator via llvm-commits]

scott.linder added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:118
 // use.
-static void buildPrologSpill(const GCNSubtarget &ST, LivePhysRegs &LiveRegs,
-                             MachineBasicBlock &MBB,
----------------
This seems to regress in terms of readability below, where instead of a call to `buildPrologSpill` we have a call to `buildPrologEpilogSpill(..., true)`, and the same for `buildEpilogReload`.

Can we name the new function `buildPrologSpillEpilogReloadImpl` and leave the old methods interface the same:

```
static void buildPrologSpill(...) { return buildPrologSpillEpilogReloadImpl(..., true); }
static void buildEpilogReload(...) { return buildPrologSpillEpilogReloadImpl(..., false); }
```

An argument against doing this may be that we already have `buildScratchExecCopy(..., true)` instead of `buildPrologScratchExecCopy` and `buildScratchExecCopy(..., false)` instead of `buildEpilogScratchExecCopy`, but I'd vote to do the same for those too.


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:831-832
 
-  if (!IsProlog)
-    LiveRegs.removeReg(ScratchExecCopy);
 
----------------
This change seems unrelated; is this the bug fix? Can it be in a separate patch after the cleanup? I'd prefer the cleanup be as close to NFC as is reasonable, although I assume there is some change in behavior because we no longer call `findScratchNonCalleeSaveRegister`, we do whatever `buildSpillLoadRestore` does.

I also might need some help understanding what is happening here to be able to review it. I'm confused about why `LivePhysRegs::init` gets called with different arguments for the prolog case vs the epilog case, and why we `stepBackward` on an (ostensibly arbitrary?) instruction after adding LiveOuts in the epilog case.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:1472
   BuildMI(MBB, MI, DL, get(Opcode))
-    .addReg(SrcReg, getKillRegState(isKill)) // data
-    .addFrameIndex(FrameIndex)               // addr
----------------
Nit: all the changes in this file can be removed from the patch


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:853-862
+    if (RS) {
       SOffset = RS->scavengeRegister(&AMDGPU::SGPR_32RegClass, MI, 0, false);
+    } else if (LiveRegs) {
+      for (MCRegister Reg : AMDGPU::SGPR_32RegClass) {
+        if (LiveRegs->available(MF->getRegInfo(), Reg)) {
+          SOffset = Reg;
+          break;
----------------
Does `RS` already know about `LiveRegs`, or something equivalent to it? Or maybe `RS` cannot fail and return `Register()`?

If not, it seems like we should continue to try alternative ways to pick an `SOffset` until we either find one or run out of ways to try.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99269/new/

https://reviews.llvm.org/D99269