[PATCH] D99269: [AMDGPU] Unify spill code

Matt Arsenault via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 31 16:12:06 PDT 2021


arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIRegisterInfo.cpp:1456-1459
+      // Use the stack pointer instead of the frame pointer for restores in the
+      // function epilog.
+      if (Flags & 1)
+        FrameReg = MFI->getStackPtrOffsetReg();
----------------
sebastian-ne wrote:
> arsenm wrote:
> > I don't like having this logic separated and overriding the frame register logic above. Can you infer this should be SP relative from the frame index itself instead of adding a new operand?
> How should that work? Should I add a new `TargetStackID::PrologEpilogSave`?
> 
> It is not really a property of the frame index that SP should be used, it is because the instruction is in the prolog/epilog where FP is not setup. If we wanted to access the same frame index in the function body, we would need to use FP instead of SP.
> 
> I could move this code out of the switch and merge it with the frame register logic above, but that doesn’t change much.
But we don't need to access these from an arbitrary point in the function. We know these indexes should only be accessed in the prolog/epilog. Fundamentally I do think this is a special case for PEI to handle. The problem here is just that what we need to do to emit spills is a lot more annoying than on other targets. 

What this probably should do is just use a common implementation function that eliminateFrameIndex and emitProlog/emitEpilog can use. buildSpillLoadStore is approximately that already. emitProlog/emitEpilog don't really need to route through the pseudos and eliminateFrameIndex


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99269



More information about the llvm-commits mailing list