This is an archive of the discontinued LLVM Phabricator instance.

[AIX] Extend integer arguments to register width when passed in stack memory.
ClosedPublic

Authored by cebowleratibm on Feb 25 2020, 8:38 AM.

Details

Summary

This is a follow up to the previous patch: [AIX] Implement caller arguments passed in stack memory. https://reviews.llvm.org/rGb373ec8ce76a00cd079f70d108b13129c3a4a7b9

This corrects a defect in AIX 64-bit where an i32 is written to the stack with stw (4 bytes) rather than the expected std (8 bytes.) Integer arguments pass on the stack as images of their register representation.

I also took the opportunity to tidy up some of the calling convention AIX tests I added in my last commit. This patch adds the missed assembly expected output for the stack arg int case, which would have caught this problem. I also took the opportunity to reorder some of the calling convention stack arg tests into a more logical order to make them more readable.

Diff Detail

Event Timeline

cebowleratibm created this revision.Feb 25 2020, 8:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 25 2020, 8:38 AM

It would be better if the test cleanup was done in a separate patch to make it clearer what the expected output of the patch. But that said, it's not difficult to find added testcase and it would be ok with me to leave the modified test cases.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6821

Suggestion: Move this error to line 6818, just after all the variable declarations required, or make this the last error (i.e. move to line 6832 and have the declarations just above it).

cebowleratibm marked an inline comment as done.Feb 26 2020, 4:42 AM

It would be better if the test cleanup was done in a separate patch to make it clearer what the expected output of the patch. But that said, it's not difficult to find added testcase and it would be ok with me to leave the modified test cases.

I agree many of the test updates could be a separate NFC patch.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6821

I preferred to keep the errors as a block. I needed to move some basic variable setup above the error block. I can move this error later in the sequence but I wouldn't move the variable setup into the error block, so this wouldn't buy anything. I prefer to keep the current approach.

Some of the prior test changes have been committed as NFC patches. I've updated the diff and readers can now see the instruction changes for widening arguments to register width before writing to the stack.

Looks good to me, thanks for updating the tests in a separate patch.

sfertile added inline comments.Mar 4 2020, 6:48 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6821

For 32-bit codegen we legalize i64s by breaking them into 2 i32s. Is i128 handled similarly, or is it a legal type which this error is trying to catch?

6822

Minor nit: word size --> register size, and maybe 'argument' --> 'integer argument'.

cebowleratibm marked 2 inline comments as done.Mar 4 2020, 7:36 AM
cebowleratibm added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6821

i128 is handled similarly to i64. In PPC64 it will take 2 registers and in PPC32 it will take 4 registers.

The code was not designed to handle this case so I put it in as a defensive measure.

One comment update. Should be ready to commit.

sfertile accepted this revision.Mar 5 2020, 7:08 AM

LGTM.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6821

Thanks for checking. In that case I would say make it an assert with a message that reads along the lines: "Illegal integer argument exceeds register size."

This revision is now accepted and ready to land.Mar 5 2020, 7:08 AM
This revision was automatically updated to reflect the committed changes.