For the fixed ABI, set this in the initial argument constructor,
rather than relying on the allocation logic to set the values. Also
stop passing them for amdgpu_gfx, since the DAG path seems to skip
these. I'm unclear on what amdgpu_gfx's expectations are. This will
allow moving the special input registers out of the normal argument
range
Details
Details
- Reviewers
rampitec madhur13490 foad Flakebi
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Also stop passing them for amdgpu_gfx, since the DAG path seems to skip these. I'm unclear on what amdgpu_gfx's expectations are.
As far as I understand, the “special inputs” are arguments relevant for HSA only and depending on EnableFixedFunctionABI they are added in the beginning or end of arguments.
So, passSpecialInputs should be guarded by “is HSA” instead of “is not amdgpu_gfx”?
I think we only want to pass the stack pointer into functions for our usage of amdgpu_gfx (and other implicit arguments like exec). All other arguments are added by the frontend.
Comment Actions
That breaks down for the other compute users too. Mesa/clover will also need most of the same inputs. Conversely, there are also inputs we don't try to handle which graphics may need (e.g. llvm.amdgcn.implicit.buffer.ptr, although I think that one is only needed in the entry point)
I think we only want to pass the stack pointer into functions for our usage of amdgpu_gfx (and other implicit arguments like exec). All other arguments are added by the frontend.
At a minimum the common ABI should defined the SP and SRD location. We also should defined the FP/BP commonly although they aren't passed (which they are now, but I'm thinking about moving them)
| llvm/lib/Target/AMDGPU/SIISelLowering.cpp | ||
|---|---|---|
| 1890 | Why do we HAVE to allocate? | |
| llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp | ||
| 67 | The constructor needs to be improved. ArgInfo will now hold allocated registers but the code ahead (line 93-99) is setting bools to true which is not useful and logically a dead code. Ideally, we should just do the necessary allocation in fixedABI and return. | |
| llvm/lib/Target/AMDGPU/SIISelLowering.cpp | ||
|---|---|---|
| 1890 | Otherwise another argument may end up allocated to the same register | |
| llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp | ||
| 67 | It's not logically dead. Whether the argument is used/needed and whether it assigned are different things. Fixed ABI is just setting the positions, and not doing the allocation. No allocation can really occur here since that's the calling convention lowering's responsibility | |
| llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp | ||
|---|---|---|
| 67 | Well, I meant that for fixed function ABI, we surely know that certain bools are going to be true, so why not set them, set the positions and return? Something like, if (UseFixedABI && CC != ...) {
PrivateSegmentBuffer = true;
DispatchId = true;
ImplicitArgPtr = true;
...
ArgInfo = AMDGPUArgumentUsageInfo::FixedABIFunctionInfo;
return;
} | |
| llvm/lib/Target/AMDGPU/SIMachineFunctionInfo.cpp | ||
|---|---|---|
| 67 | We do that below. There are some problems with the interaction between amdgpu_gfx and the input setting I'm going to fix in a separate patch | |
This comment does make more sense in the function above, where the register is allocated in the CCInfo.