Details
- Reviewers
uweigand - Commits
- rG22f9429149a8: [SystemZ] Add GHC calling convention
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
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 |
- 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.
- Beside that I changed every assert(e) into an if(e) report_fatal_error(e')
- 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)?
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.
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.