[PATCH] D81364: AMDGPU Initializes StackPtrOffset when NonSpillStackObjects present

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

Sorry for the delay in reviewing, I've tried to wrap my head around this and I'm still lost.

I think someone needs to define what all of these things mean. Some fundamental questions I don't think we have really adequately answered are:

- What is the SP, when is it defined, when can it be changed?
- What is the FP, when is it defined, when can it be changed?
- Can we decide ahead-of-time (ISel?) whether we will need to have an SGPR available for the immediate-overflow case in `SIRegisterInfo::buildSpillLoadStore`?
  - Should we be using the SP/FP to handle that case in the first place (temporarily munging the SP/FP has implications for e.g. trap handlers, signals, CFI, etc.)?
  - If not, should we add an additional SGPR argument to the spill or something similar to ensure there is an SGPR available so we don't just ICE in the case where one isn't? Is this possible with the way RA works?
  - Can we in some cases prove that the spare SGPR is not needed because we will be able to fit the offset into the immediate field of the buffer instruction? Can we actually leverage this to free up that SGPR to be used by RA, or is this a circular problem/impossible because of potential need for the SGPR much later? Is this part of the purpose of the emergency spill slot? Is this an issue because we can't actually spill SGPRs directly to VMEM, and the emergency slot is in VMEM?

It seems like both of these patches, while maybe fixing some specific cases, aren't really framing the solution in terms of what is fundamentally broken. I think I broke this further with my patch to add the scratch wave offset to the scratch SRD, but I think all of these issues existed before. I don't know what the right answer is, but I can't really review anything without at least that amount of context.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81364