[PATCH] D69024: [SystemZ] Add GHC calling convention

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

Just for my understanding:  You have redefined the way arguments are passed with the GHC convention, but **return values** are still implemented the same way as with the default convention?

This has the somewhat unexpected effect that the return value is no longer in the same register as the first parameter, like it is with most conventions.  The usual convention has the advantage that a function "int f(int x) { return x; }" can be implemented without any code (just a "br %r14").

Now, to be clear, it is of course possible to implement a convention that doesn't have this property, I just wanted to verify that is indeed what was intended.



================
Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.cpp:1303
+  default:
+    report_fatal_error("Unsupported calling convention");
+  }
----------------
This may now cause a regression for some calling conventions that could in theory be used (e.g. CallingConv::Cold or CallingConv::CXX_FAST_TLS).   I think it would be preferable to just default to the standard calling convention instead.


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.cpp:1692
 
+  assert(CallConv != CallingConv::GHC && "GHC functions return void only.");
+
----------------
This should probably be a report_fatal_error instead of an assert, since the situation can be triggered by (unsupported) LLVM IR input files, and not by a bug in LLVM.  assert really is only intended to catch LLVM coding bugs.


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.cpp:2886
+             CallingConv::GHC &&
+         "In GHC calling convention TLS is not supported.");
+
----------------
Likewise here and below.


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelLowering.cpp:3888
   MF.getInfo<SystemZMachineFunctionInfo>()->setManipulatesSP(true);
+  assert(MF.getFunction().getCallingConv() != CallingConv::GHC);
   return DAG.getCopyFromReg(Op.getOperand(0), SDLoc(Op),
----------------
And here (and below) as well, I think.   This situation can be triggered by LLVM IR using variable-sized stack allocation, which I guess is not supported with the GHC calling convention.


================
Comment at: llvm/test/CodeGen/SystemZ/ghc-cc-02.ll:2
+; RUN: llc -mtriple=s390x-ibm-linux < %s | FileCheck %s
+; XFAIL: *
+
----------------
If you want to verify that a certain error is raised, using XFAIL is not the right way.  Instead, you need to write the test so that it **passes** if llc fails.  This is usually done by using the "not" tool as part of the RUN command line, see e.g. vec-args-error-01.ll


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69024