[PATCH] D88540: [AMDGPU] Add amdgpu_gfx_callable calling convention

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

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



================
Comment at: llvm/include/llvm/IR/CallingConv.h:245
+    /// Calling convention used for AMDPAL callable shaders.
+    AMDGPU_GFX_CALLABLE = 100,
+
----------------
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


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCallLowering.cpp:462-463
   CallingConv::ID CC = B.getMF().getFunction().getCallingConv();
   const bool IsShader = AMDGPU::isShader(CC);
+  const bool IsCallableShader = CC == CallingConv::AMDGPU_GFX_CALLABLE;
   const bool IsWaveEnd = (IsShader && MFI->returnsVoid()) ||
----------------
I think shaders should refer to the entry points, so IsCallableShader wouldn't make sense as a name


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:3120
+  if (!AMDGPUTargetMachine::EnableFixedFunctionABI &&
+      !Subtarget->isAmdPalOS()) {
     // Copy special input registers after user input arguments.
----------------
This should probably be a positive hsa check


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp:1014
     case CallingConv::AMDGPU_CS:
+    case CallingConv::AMDGPU_GFX_CALLABLE:
       return true;
----------------
I think this should return false, it's not an entry point


================
Comment at: llvm/lib/Target/AMDGPU/Utils/AMDGPUPALMetadata.cpp:737
+  case CallingConv::AMDGPU_GFX_CALLABLE:
+    assert(false && "Callable shader has no hardware stage");
   default:
----------------
llvm_unreachable


================
Comment at: llvm/test/CodeGen/AMDGPU/unsupported-calls.ll:72
 
-; GCN: :0:0: in function test_call_from_shader i32 (): unsupported call from graphics shader of function defined_function
+; GCN-NOT: :0:0: in function test_call_from_shader i32 (): unsupported call from graphics shader of function defined_function
 ; R600: in function test_call{{.*}}: unsupported call to function defined_function
----------------
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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88540