This is an archive of the discontinued LLVM Phabricator instance.

[AMDGPU] Add amdgpu_gfx calling convention
ClosedPublic

Authored by Flakebi on Sep 30 2020, 1:37 AM.

Details

Summary

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.

Diff Detail

Event Timeline

Flakebi created this revision.Sep 30 2020, 1:37 AM
Flakebi requested review of this revision.Sep 30 2020, 1:37 AM
foad added a subscriber: foad.Sep 30 2020, 4:04 AM

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
462–463

I think shaders should refer to the entry points, so IsCallableShader wouldn't make sense as a name

llvm/lib/Target/AMDGPU/SIISelLowering.cpp
3120

This should probably be a positive hsa check

llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
1014

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)

Flakebi updated this revision to Diff 299337.Oct 20 2020, 4:54 AM

Update from internal review comments.

Rename calling convention to amdgpu_gfx.

Flakebi updated this revision to Diff 299346.Oct 20 2020, 5:36 AM
Flakebi marked 5 inline comments as done.

Disallow calls with C calling convention from shaders

Flakebi added inline comments.Oct 20 2020, 5:38 AM
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
2078–2080

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–476

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
2957–2963

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 ↗(On Diff #299346)

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?

nhaehnle added inline comments.Oct 22 2020, 7:59 AM
llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
5090 ↗(On Diff #299346)

Okay, technically I suppose it shouldn't break radeonsi, but regress it.

Flakebi updated this revision to Diff 300203.Oct 23 2020, 2:53 AM
Flakebi marked 4 inline comments as done.

Fix comments

llvm/lib/Target/AMDGPU/SIISelLowering.cpp
2957–2963

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).

Flakebi retitled this revision from [AMDGPU] Add amdgpu_gfx_callable calling convention to [AMDGPU] Add amdgpu_gfx calling convention.Oct 26 2020, 9:14 AM
Flakebi edited the summary of this revision. (Show Details)
Flakebi updated this revision to Diff 300954.Oct 27 2020, 4:53 AM

Fix lld test failure

arsenm added a comment.Nov 3 2020, 7:05 AM

Needs globalisel tests

llvm/lib/Target/AMDGPU/SIISelLowering.cpp
1787–2258

Missing equivalent GlobalISel changes

sebastian-ne added inline comments.
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
1787–2258

What would that be?
The change here shouldn’t change behavior apart from checking the assert for amdgpu_gfx and I can’t find the same structure in AMDGPUCallLowering.cpp.

arsenm added inline comments.Nov 4 2020, 9:13 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
1787–2258

Should at least add the tests or run line for it

Fixed a bug for GlobalISel and add tests.
Thanks for pushing for tests Matt.

arsenm accepted this revision.Nov 6 2020, 11:34 AM
This revision is now accepted and ready to land.Nov 6 2020, 11:34 AM
This revision was landed with ongoing or failed builds.Nov 9 2020, 7:52 AM
This revision was automatically updated to reflect the committed changes.
llvm/test/Bitcode/compatibility.ll