[PATCH] D99269: [AMDGPU] Unify spill code

[Sebastian Neubauer via Phabricator via llvm-commits]

sebastian-ne added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:118
 // use.
-static void buildPrologSpill(const GCNSubtarget &ST, LivePhysRegs &LiveRegs,
-                             MachineBasicBlock &MBB,
----------------
scott.linder wrote:
> sebastian-ne wrote:
> > 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?
> > There are some other functions (SIRegisterInfo::buildSpillLoadStore and SIRegisterInfo::buildSGPRSpillLoadStore) that also take a boolean argument to switch between load and store.
> 
> I think some amount of this is pragmatic and reasonable, I just start to find it harder to read when it gets used broadly enough. Also in the case of `buildSpillLoadStore` the boolean is capturing whether to `kill` the operand, although it seems like that would always be equivalent to whether `Opcode` is a load or a 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.
> 
> This LGTM, thank you! The differences between the two are more obvious to me now than in the conditional, combined version.
> 
> > I added comments to buildScratchExecCopy, does that look ok or should I add wrappers for prolog and epilog?
> 
> Sorry, I only meant to justify my comments on your code, I didn't mean to ask for a change as part of this patch. The comment LGTM.
> > There are some other functions (SIRegisterInfo::buildSpillLoadStore and SIRegisterInfo::buildSGPRSpillLoadStore) that also take a boolean argument to switch between load and store.
> 
> I think some amount of this is pragmatic and reasonable, I just start to find it harder to read when it gets used broadly enough. Also in the case of `buildSpillLoadStore` the boolean is capturing whether to `kill` the operand, although it seems like that would always be equivalent to whether `Opcode` is a load or a store?

Whoops, you’re right, `buildSpillLoadStore` directly takes the opcode instead of a boolean. I think `kill` can be false on a store if the register is still used afterwards.


================
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:
> sebastian-ne wrote:
> > 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.
> Makes sense, but in that case could you add an `assert(!RS != !LiveRegs && "expect either RS or LiveRegs but not both")`?
I think it’s supported to call `buildSpillLoadStore` without a register scavenger (and without LiveRegs), that’s why the check for `RS` was here before, so I added an assert for `!RS || !LiveRegs`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99269