[PATCH] D124192: [AMDGPU] Callee must always spill writelane VGPRs

Wed Jun 29 02:43:12 PDT 2022

nhaehnle added inline comments.

================
Comment at: llvm/lib/Target/AMDGPU/SIFrameLowering.cpp:1263
+      if (MI.getOpcode() == AMDGPU::V_WRITELANE_B32)
+        MFI->addToLaneVGPRs(MF, MI.getOperand(0).getReg());
+    }
----------------
arsenm wrote:
> cdevadas wrote:
> > arsenm wrote:
> > > cdevadas wrote:
> > > > arsenm wrote:
> > > > > I don't think this is the right place to determine these registers. It's adding an extra loop over the function, and adding statefulness to determineCalleeSaves. Theoretically we should be able to call it multiple times 
> > > > Any recommendation?
> > > > This should be done before identifying the CSR registers and they should be skipped from the default CSR spill insertion routines.
> > > Logically I think this belongs in PEI::calculateCallFrameInfo, but this isn't really a reasonable thing to add to the generic code.
> > Yes, we can't accommodate the code into PEI::calculateCallFrameInfo.  The processFunctionBeforeFrameFinalized is also not a viable option. At this moment, determineCalleeSaves seemed a better place. The need to iterate over the MBB, in this case, can't be avoided.
> I don't like it, but I guess we can go with this for now. Is there a way to assert that this was only computed once? I know at one point I had a patch that tried to speculatively call this to see what spills will be later inserted
I don't understand why this is necessary in the first place. Can't you call `allocateWWMSpill` when the V_WRITELANE_B32 for spilling is inserted? Actually, isn't that already happening?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124192