[prev in list] [next in list] [prev in thread] [next in thread] 

List:       llvm-commits
Subject:    [PATCH] D99269: [AMDGPU] Unify spill code
From:       Matt Arsenault via Phabricator via llvm-commits <llvm-commits () lists ! llvm ! org>
Date:       2021-03-31 23:12:06
Message-ID: eNjxktqLTM2m6d4aNbMaOw () ismtpd0156p1iad2 ! sendgrid ! net
[Download RAW message or body]

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

_______________________________________________
llvm-commits mailing list
llvm-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits


[prev in list] [next in list] [prev in thread] [next in thread]