This patch reworks the AIX call lowering to use CCState. Some defensive errors are added in this patch to protect from emitting bad code for calling convention logic that has not been implemented by design. The use of CCState follows the precedent of other targets and enables the reuse of calling convention logic in LowerFormalArguments, which will be rewritten to also use CCState in a later patch.
Details
Diff Detail
Event Timeline
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
6729 | report_fatal_error is more appropriate here. The default case isn't really unreachable. | |
6783 | Minor nit: 'Unknown' --> Unexpected. | |
6794 | minor nit: QPX is not supported on AIX. | |
6818 | You should be taking the max between LinkageSize + MinParameterSaveAreaSize and the stack size determined by the CCState object. |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
6818 | Agreed but the patch is meant to be NFCI and the current code doesn't support arguments on the stack so this has the same semantics as the prior code. A future patch will address your concern. |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
6705 | We should probably have some simple lit tests for these fatal errors. At least byval/sret/nest. | |
6818 | But CC_AIX also doesn't handle arguments on the stack so IIUC the largest offset you could get back is LinkageSize + MinParameterSaveAreaSize keeping this patch NFC. FWIW I am OK with you leaving it out for now, I don't think its worth holding up the patch over. We should have a lit tests that tests overflow args though that shows the error. |
I'll add some test coverage for some of the limitation errors.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
6818 | Fair enough, it would still be NFC but have the same semantic. My intention is to correct this in a coming patch that adds support for stack arguments. It seems to logically fit with that work. |
Add tests for the limitation errors on sret parameters, byval parameters as well as arguments passed in memory. These are limitations that will be addressed in future work.
One last test for error on AIX nest parameters. I renamed some of the aix tests for consistency.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
6727 | Nit: I know in the caller side of CC functions, we always pass in the same argument for both LocVT and ValVT. But to be more pedantic and for both paramaters intended usage, should we check the ValVT here instead? | |
6821 | nit: Add '.' for the sentence. | |
llvm/test/CodeGen/PowerPC/aix-byval-param.ll | ||
2 | add -verify-machine-instr, -mcpu to all the llc command? |
llvm/test/CodeGen/PowerPC/aix-byval-param.ll | ||
---|---|---|
2 | For the calling conventions we've been using -stop-after=machine-cp, so no need for -verify-machine-instr etc at this point. That said, I guess these testcase are here for to generate the error but it would be useful if the RUN command was reflected what we usually do, that way we make sure the tests are uniform if/when we enable these. |
llvm/lib/Target/PowerPC/PPCISelLowering.cpp | ||
---|---|---|
6727 | It's more intuitive to the reader, though on AIX ABI LocVT and ValVT will always come in with the same value. I'll make the suggested update. | |
llvm/test/CodeGen/PowerPC/aix-byval-param.ll | ||
2 | I'm not adding -verify-machine-instr because of -stop-after=machine-cp These CC tests are not sensitive to cpu level because the ABI must be consistent across all AIX supported cpu. One thought is to explicitly run the CC tests in all -mcpu, however, I think that's overkill. |
LGTM.
llvm/test/CodeGen/PowerPC/aix-byval-param.ll | ||
---|---|---|
2 | I see. Missed the fact it is testing MIR. Thanks. |
We should probably have some simple lit tests for these fatal errors. At least byval/sret/nest.