Add a calling convention called amdgpu_gfx for real function calls
within graphics shaders. For the moment, this uses the same calling
convention as other calls in amdgpu, with registers excluded for return
address, stack pointer and stack buffer descriptor.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
One problem is you seem to be assuming this is the one calling convention usable from both CS and other shader types. However, the default FP mode is assumed to be different between these (we assume IEEE mode is enabled for CS and not for other shader types). These would all need to be agree to avoid inserting mode switches.
I also don't see any tests for the inreg SGPR arguments. I think this is the most difficult part to handle since there isn't a straightforward way to insert the necessary waterfall loops, so it probably will require many later patches.
Can you also add a note to the AMDGPUUsage section since this is a different ABI with different register counts
llvm/include/llvm/IR/CallingConv.h | ||
---|---|---|
245 | I don't like the name. "Callable" doesn't feel like it belongs in the name. amdgpu_gfx_func would be better, but I don't think that's great either | |
llvm/lib/Target/AMDGPU/AMDGPUCallLowering.cpp | ||
461–462 | I think shaders should refer to the entry points, so IsCallableShader wouldn't make sense as a name | |
llvm/lib/Target/AMDGPU/SIISelLowering.cpp | ||
3112 | This should probably be a positive hsa check | |
llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp | ||
1040 | I think this should return false, it's not an entry point | |
llvm/lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp | ||
737 | llvm_unreachable | |
llvm/test/CodeGen/AMDGPU/unsupported-calls.ll | ||
72 | This is very fragile negative check. It's also questionable if this should be allowed to call C calling conventions. It would need an FP mode switch since the default IEEE mode bit is different (although it's currently not the backend's responsibility to insert it) |
llvm/test/CodeGen/AMDGPU/unsupported-calls.ll | ||
---|---|---|
72 | C calling convention is now disallowed. I made the check more generic but I didn’t find a great way to make it a positive check that would not break checks for the other test functions. |
I noticed what I believe is one more correctness issues, plus a few assorted comments.
llvm/lib/AsmParser/LLParser.cpp | ||
---|---|---|
2089–2091 | This could be aligned with the rest of the case statements again now. | |
llvm/lib/IR/AsmWriter.cpp | ||
391–393 | Same here wrt alignment to the other case statements. | |
llvm/lib/Target/AMDGPU/AMDGPUCallLowering.cpp | ||
473–474 | IsGraphics is true for amdgpu_gfx functions, right? Why are we using SI_RETURN_TO_EPILOG in this case? | |
llvm/lib/Target/AMDGPU/SIISelLowering.cpp | ||
2948–2954 | I assume you'll add support for calls from amdgpu_cs to amdgpu_gfx in a follow-up change? | |
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp | ||
5090 | I believe this will break radeonsi because it actually ends up settings OS == unknown. We'll have to follow-up on that, but in the meantime, couldn't this use AMDGPU::isGraphics? |
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp | ||
---|---|---|
5090 | Okay, technically I suppose it shouldn't break radeonsi, but regress it. |
Fix comments
llvm/lib/Target/AMDGPU/SIISelLowering.cpp | ||
---|---|---|
2948–2954 | Calls from any shader type to amdgpu_gfx should be allowed with this. Calls with any calling convention that is not amdgpu_gfx will fail. “Fail” in this case means it silently prints a message if output is enabled and compiles successful anyway (just omitting the call). |
Needs globalisel tests
llvm/lib/Target/AMDGPU/SIISelLowering.cpp | ||
---|---|---|
1783–2266 | Missing equivalent GlobalISel changes |
llvm/lib/Target/AMDGPU/SIISelLowering.cpp | ||
---|---|---|
1783–2266 | What would that be? |
llvm/lib/Target/AMDGPU/SIISelLowering.cpp | ||
---|---|---|
1783–2266 | Should at least add the tests or run line for it |
I don't like the name. "Callable" doesn't feel like it belongs in the name. amdgpu_gfx_func would be better, but I don't think that's great either