[PATCH] D124195: [AMDGPU] Separate out SGPR spills to VGPR lanes during PEI

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:1295-1301
+  // Mark all lane VGPRs as BB LiveIns.
+  for (MachineBasicBlock &MBB : MF) {
+    for (auto &Reg : MFI->getWWMSpills())
+      MBB.addLiveIn(Reg.first);
+
+    MBB.sortUniqueLiveIns();
+  }
----------------
cdevadas wrote:
> arsenm wrote:
> > Actually, do we really need to do this anymore? If they were allocated from virtual registers, they should have correct livens lists already 
> They are needed for prolog/epilog spill insertion. If we don't mark them liveIn, there will be a MIR verifier error indicating the use of undefined registers in spill instructions.
> 
This feels too coarse grain. The whole point of doing this was to allocate these like normal virtual registers, which should then have naturally set liveins already. Is this only handling the prolog/epilog cases? It should only need to do anything for those 


================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp:309-316
+      // FIXME: We can run out of free registers with split allocation if
+      // IPRA is enabled and a called function already uses every VGPR.
+#if 0
+        DiagnosticInfoResourceLimit DiagOutOfRegs(MF.getFunction(),
+                                                  "VGPRs for SGPR spilling",
+                                                  0, DS_Error);
+        MF.getFunction().getContext().diagnose(DiagOutOfRegs);
----------------
I think this referenced error cannot happen anymore


================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp:360
+bool SIMachineFunctionInfo::allocateSGPRSpillToVGPRLane(MachineFunction &MF,
+                                                        int FI, bool IsPEI) {
+  std::vector<SIRegisterInfo::SpilledReg> &SpillLanes =
----------------
IsPEI feels like the wrong name. IsPrologEpilog would be a bit better but not great


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124195