[PATCH] D99269: [AMDGPU] Unify spill code

[Sebastian Neubauer via Phabricator via llvm-commits]

sebastian-ne added a comment.

Thanks for your comments Scott, I tried to address all of them.



================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:118
 // use.
-static void buildPrologSpill(const GCNSubtarget &ST, LivePhysRegs &LiveRegs,
-                             MachineBasicBlock &MBB,
----------------
scott.linder wrote:
> 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.
There are some other functions (`SIRegisterInfo::buildSpillLoadStore` and `SIRegisterInfo::buildSGPRSpillLoadStore`) that also take a boolean argument to switch between load and store. Now that I look at buildPrologEpilogSpill again, it’s quite short and the load/spill parts are different enough, so it makes sense to split them again.

I added comments to `buildScratchExecCopy`, does that look ok or should I add wrappers for prolog and epilog?


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:831-832
 
-  if (!IsProlog)
-    LiveRegs.removeReg(ScratchExecCopy);
 
----------------
scott.linder wrote:
> 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.
I split out the fix into D100098.
I think the difference is, in the prolog we start in the beginning and track register liveness to the current insertion point (MBBI). In the epilog we start at the end and track liveness to the current insertion point. The result should be the same, but the epilog is probably at the end of the basic block while the prolog is at the beginning, so it’s cheaper to start from the end/start.


================
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;
----------------
scott.linder wrote:
> 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.
Usually, `buildSpillLoadStore` is called to lower spill pseudos. At that point, we get a RegScavenger. During frame lowering (which did not call this function before), we do not have access to a RegScavenger as far as I know, so we use LiveRegs to find an unused register.
So, when calling this function, either `RS` should be set (during frame index elimination), or `LiveRegs` (during frame lowering), but never both, as they do the same thing.

`RS` can fail if all SGPRs are live. Usually it would then try to spill a register to the emergency spill slot, but that doesn’t make sense if we are currently lowering a spill, so we forbid that with the `false` last argument.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99269