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

[Christudasan Devadasan via Phabricator via llvm-commits]

cdevadas 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();
+  }
----------------
arsenm wrote:
> cdevadas wrote:
> > arsenm wrote:
> > > 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 
> > Yes, they are needed only for prolog/epilog spill cases.
> But getWWMSpills covers everything? this is adding excess live ins?
It doesn't necessarily add the live-ins at the entry block. We insert the spill to a virt-VGPR at a block properly adding the IMPLICIT_DEF at its dominator block. The physical VGPR allocated for this virt-VGPR should be added to the prolog block live-ins otherwise verifier would complain about its spill store for using an undefined register.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124195