[PATCH] D153517: [AMDGPU] ISel for amdgpu_cs_chain[_preserve] functions

[Diana Picus via Phabricator via llvm-commits]

rovka added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp:91
+    // right after the arguments.
+    StackPtrOffsetReg = AMDGPU::SGPR105;
+
----------------
nhaehnle wrote:
> rovka wrote:
> > arsenm wrote:
> > > This is an odd choice of value and it doesn't exist on all subtargets
> > I tried setting the SP in `finalizeLowering` using basically [[ https://github.com/llvm/llvm-project/blob/5cd16e2cb7207a70a61d1221464b17dcaf3c7f29/llvm/lib/Target/AMDGPU/SIISelLowering.cpp#L2380 | this snippet ]], but it blew up for Global ISel (I think because of some issue with the liveins). I figured I should ask here before going too far down that rabbit hole - does that sound like the right approach, or is there a better place to handle the SP? I'm guessing there are other things I could prioritize before fixing this, but do let me know if you think otherwise.
> Realistically, we're only ever going to use this on gfx10+, where the register always exists. If you're too worried, we could have a corresponding assertion.
> 
> That said, it's not clear to me what this value can end up being used for, since chain functions don't have a stack pointer input or output. A stack pointer should be set up if we call an amdgpu_gfx function; does it matter whether the chosen register is aligned with that calling convention?
> Realistically, we're only ever going to use this on gfx10+, where the register always exists. If you're too worried, we could have a corresponding assertion.

Ok, I could add that.

> That said, it's not clear to me what this value can end up being used for, since chain functions don't have a stack pointer input or output. A stack pointer should be set up if we call an amdgpu_gfx function; does it matter whether the chosen register is aligned with that calling convention?

Honestly I hadn't thought much about that, I was hellbent on putting it right after the arguments because there seems to be some precedent for that (and code already written; it's nice when code is already written). I'm going to ruminate some more on whether we can just use s32.


================
Comment at: llvm/test/CodeGen/AMDGPU/amdgpu-cs-chain-preserve-cc.ll:16
+
+define amdgpu_cs_chain_preserve void @amdgpu_cs_chain_preserve_cc(<4 x i32> inreg %a, <4 x i32> %b) {
+  ; GISEL-GFX11-LABEL: name: amdgpu_cs_chain_preserve_cc
----------------
nhaehnle wrote:
> These functions probably need way more scratch save/restore. Actually, I'm starting to suspect that we can't reliably code-generate amdgpu_cs_chain_preserve functions that call amdgpu_gfx functions, because the amdgpu_gfx calling convention has some caller-saves registers, which just doesn't mesh well.
> 
> This probably needs a bit more thought, though it shouldn't be a big problem in practice: the purpose / intended use of the _preserve calling convention is very narrow.
I'm not sure I understand your comment, I assume you mean it needs more scratch save/restore so it actually preserves things, like the name says? :) If that's what you had in mind, isn't that PEI's job? (I've only just started looking into that, since I thought it's not ISel's job - but I could be wrong)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153517