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

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

I noticed what I believe is one more correctness issues, plus a few assorted comments.



================
Comment at: llvm/lib/AsmParser/LLParser.cpp:2089-2091
+  case lltok::kw_amdgpu_gfx:
+    CC = CallingConv::AMDGPU_Gfx;
+    break;
----------------
This could be aligned with the rest of the case statements again now.


================
Comment at: llvm/lib/IR/AsmWriter.cpp:391-393
+  case CallingConv::AMDGPU_Gfx:
+    Out << "amdgpu_gfx";
+    break;
----------------
Same here wrt alignment to the other case statements.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUCallLowering.cpp:473-474
 
   unsigned ReturnOpc =
-      IsShader ? AMDGPU::SI_RETURN_TO_EPILOG : AMDGPU::S_SETPC_B64_return;
+      IsGraphics ? AMDGPU::SI_RETURN_TO_EPILOG : AMDGPU::S_SETPC_B64_return;
 
----------------
IsGraphics is true for amdgpu_gfx functions, right? Why are we using SI_RETURN_TO_EPILOG in this case?


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:2948-2954
+  if (AMDGPU::isShader(MF.getFunction().getCallingConv()) &&
+      CallConv != CallingConv::AMDGPU_Gfx) {
+    // Only allow calls with specific calling conventions.
+    return lowerUnhandledCall(CLI, InVals,
+                              "unsupported calling convention for call from "
+                              "graphics shader of function ");
   }
----------------
I assume you'll add support for calls from amdgpu_cs to amdgpu_gfx in a follow-up change?


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.cpp:5090
   if (isMIMG(MI) ||
-      (AMDGPU::isShader(MF.getFunction().getCallingConv()) &&
-       (isMUBUF(MI) || isMTBUF(MI)))) {
+      ((ST.isAmdPalOS() || ST.isMesa3DOS()) && (isMUBUF(MI) || isMTBUF(MI)))) {
     MachineOperand *SRsrc = getNamedOperand(MI, AMDGPU::OpName::srsrc);
----------------
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88540