This is an archive of the discontinued LLVM Phabricator instance.

[AIX] Refactor AIX Call Lowering to use CCState. NFCI.
ClosedPublic

Authored by cebowleratibm on Oct 17 2019, 4:56 AM.

Details

Summary

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.

Diff Detail

Event Timeline

cebowleratibm created this revision.Oct 17 2019, 4:56 AM
sfertile added inline comments.Oct 21 2019, 6:57 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6729

report_fatal_error is more appropriate here. The default case isn't really unreachable.
Minor nit: Unexpected --> Unhandled.

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.

cebowleratibm marked an inline comment as done.

Address nit comments.

cebowleratibm marked 2 inline comments as done.

Minor correction on one of the nit comments.

cebowleratibm marked 2 inline comments as done.Oct 22 2019, 4:55 AM
cebowleratibm added inline comments.
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.

sfertile added inline comments.Oct 22 2019, 8:41 AM
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.

cebowleratibm planned changes to this revision.Oct 22 2019, 10:33 AM
cebowleratibm marked 2 inline comments as done.

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.

jasonliu added inline comments.Oct 23 2019, 11:26 AM
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
1 ↗(On Diff #226136)

add -verify-machine-instr, -mcpu to all the llc command?

ZarkoCA added inline comments.Oct 23 2019, 6:26 PM
llvm/test/CodeGen/PowerPC/aix-byval-param.ll
1 ↗(On Diff #226136)

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.

cebowleratibm marked 2 inline comments as done.Oct 24 2019, 9:03 AM
cebowleratibm added inline comments.
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
1 ↗(On Diff #226136)

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.

Use ValVT in CC_AIX rather than LocVT.

Added a period to a comment sentence.

jasonliu accepted this revision.Oct 24 2019, 9:19 AM

LGTM.

llvm/test/CodeGen/PowerPC/aix-byval-param.ll
1 ↗(On Diff #226136)

I see. Missed the fact it is testing MIR. Thanks.

This revision is now accepted and ready to land.Oct 24 2019, 9:19 AM
This revision was automatically updated to reflect the committed changes.