This is an archive of the discontinued LLVM Phabricator instance.

[SystemZ] Add GHC calling convention
ClosedPublic

Authored by stefansf on Oct 16 2019, 2:07 AM.

Diff Detail

Event Timeline

stefansf created this revision.Oct 16 2019, 2:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 16 2019, 2:07 AM

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.

llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
1303

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.

1678

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.

2882

Likewise here and below.

3884

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.

llvm/test/CodeGen/SystemZ/ghc-cc-02.ll
3

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

stefansf updated this revision to Diff 225753.Oct 19 2019, 5:20 AM

Incorporate changes from review

stefansf marked 4 inline comments as done.Oct 19 2019, 5:31 AM
  1. One of the test cases was miss leading in the sense that it simulated a return value which does not exist in GHC calling convention, i.e., every GHC function call is a non-returning tail call. I changed ghc-cc-01.ll and removed the miss leading test case.
  1. Beside that I changed every assert(e) into an if(e) report_fatal_error(e')
  1. I removed function CCAssignFnForCall and introduced CCIfCC in CC_SystemZ. This should preserve the current semantics and only add the GHC calling convention.

Thank you very much for your detailed review so far!

OK, looks good to me now. Maybe just for completeness sake, could you add test cases verifying the other errors are emitted as expected (non-void return, variable-sized stack allocations, TLS use)?

stefansf updated this revision to Diff 226097.Oct 23 2019, 12:45 AM

More test cases added

While adding a test case for each report_fatal_error() I also added a check for
a frame pointer in function emitPrologue.

One could argue whether test case ghc-cc-07.ll checks for a frame pointer or
for a variable length array. I checked and reported for the former since a VLA
requires a frame pointer, however, the pure existence of a frame pointer does
not imply a VLA. Thus checking for a frame pointer---which is not supported in
GHC CC since register r11 is occupied---is more general and catches hopefully
more potential errors.

uweigand accepted this revision.Nov 4 2019, 4:28 AM

LGTM now, thanks!

This revision is now accepted and ready to land.Nov 4 2019, 4:28 AM
This revision was automatically updated to reflect the committed changes.