Page MenuHomePhabricator

[AIX] Add support for non var_arg extended vector ABI calling convention on AIX
ClosedPublic

Authored by ZarkoCA on Aug 24 2020, 10:33 AM.

Details

Summary

This patch enables passing non variadic vector type parameters on the caller and callee side and vector return on AIX that are passed in vector registers only.

So far, support is enabled for only the AIX extended Altivec ABI Calling convention.

Diff Detail

Event Timeline

ZarkoCA created this revision.Aug 24 2020, 10:33 AM
ZarkoCA requested review of this revision.Aug 24 2020, 10:33 AM
ZarkoCA edited the summary of this revision. (Show Details)Aug 24 2020, 10:56 AM
ZarkoCA planned changes to this revision.Sep 24 2020, 6:09 AM

Going to work on rebasing this patch.

sfertile added inline comments.Sep 28 2020, 7:25 AM
llvm/lib/Target/PowerPC/PPCSubtarget.h
329 ↗(On Diff #287442)

Can I ask why this got modified?

ZarkoCA marked an inline comment as done.Sep 28 2020, 7:35 AM
ZarkoCA added inline comments.
llvm/lib/Target/PowerPC/PPCSubtarget.h
329 ↗(On Diff #287442)

I was experimenting to see what differences that would make to some test cases I was trying. It shouldn't be in this patch. Sorry for the confusion.

Hi Zarko, could rebase this patch on the latest master?

ZarkoCA marked an inline comment as done.Sep 28 2020, 9:28 AM

Hi Zarko, could rebase this patch on the latest master?

Yes, the patch is in the changes planned state. I will rebase when I put it up again.

We are in a poor position since this depends on D75059 which I never commit-ed since as I realized it was not really NFC because we were wrongly inserting VR_SAVE code for AIX. This had the effect of emitting a fatal error if we used vector arguments, return values and I believe if we selected any vector instructions. I never went back and updated it because there was always something higher priority to work on. I have a non-NFC update I haven't posted which relies on the calling convention lowering to emit an error when passing vector arguments, and I add an error when returning vector arguments. I will post that shortly.

This patch is trying implement 2 related but distinct things

  1. Passing vector arguments.
  2. Spilling and restoring of the vector.

The test for 1 will have a problem since the dependent patch will change to emitting an error for the return of a vector type. The second thing has no test coverage in this test. I suggest we put this patch on hold, and then proceed with:

  1. I post the updated VRSAVE removal patch, which we can review and get commited first.
  2. You split out the frame lowering changes from this patch and test them separately. We should be able to land that once D75059 lands.

Then we can either rework the lit test here to not return vector types and try to land this patch first, or enable vector returns (I think it will work as it, but we need lit test coverage).

ZarkoCA updated this revision to Diff 296182.Oct 5 2020, 7:50 AM
ZarkoCA retitled this revision from [AIX] Add support for vector parameters in LowerFormalArguments_AIX to [AIX] Add support for non var_arg extended vector ABI calling convention on AIX.
ZarkoCA edited the summary of this revision. (Show Details)
ZarkoCA added a reviewer: Xiangling_L.

This patch now only contains the ISel lowering portion of the previous version. Additionally, lowering on the callee side was implemented since with the work already contained in CC_AIX only the removal of the error in LowerCall_AIX() was required. Return vector types was also enabled. The test case checks the caller, callee and return assembly instructions.

Hi Zarko, the patch is out of sync with master branch, could you update it?

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

Just want to confirm that this would not lead to a warning when compiled with assertion off?

6948

If I understand correctly, isByVal() section is for aggregate parameter types only? Can we add some comments or even better an assertion here to indicate that?

7048

Are we using v2i64 to support vector long? As ABI says, the vector type with the long keyword are deprecated. Should we emit an error for this type instead?

ZarkoCA updated this revision to Diff 298472.Oct 15 2020, 2:34 PM

Rebased based on new changes in https://reviews.llvm.org/D88676.
Added error for vector long type.

ZarkoCA marked an inline comment as done.Oct 15 2020, 2:41 PM
ZarkoCA added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6925

I think so, but I wasn't looking to mess with that assert only to avoid it when using vectors.

6948

Yes, that sections only handles arguments that have the ByVal flag.

I think adding comments or an assertion is not related to this patch. We would need a separate review to handle that.

7048

Good catch, thanks.

ZarkoCA updated this revision to Diff 298995.Oct 19 2020, 3:42 AM
ZarkoCA marked an inline comment as done.
ZarkoCA edited the summary of this revision. (Show Details)

Addressed comments and adding parent revisions.

ZarkoCA updated this revision to Diff 305202.Nov 13 2020, 9:54 AM

Rebase to reflect changes in parent revisions.

DiggerLin accepted this revision.Nov 19 2020, 6:04 AM
DiggerLin added a subscriber: DiggerLin.

LGTM , but I think it need other person to approve it before commit

This revision is now accepted and ready to land.Nov 19 2020, 6:04 AM
sfertile added inline comments.Nov 19 2020, 10:03 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6984

I don;'t think we can determine if a v2i64 came from a vector long in the IR since vector long long is a v2i64 as well. Any error handling for the altivec type would have to be done in the front-end I believe.

ZarkoCA updated this revision to Diff 307610.Nov 25 2020, 7:35 AM

Removed vector long error

ZarkoCA marked 3 inline comments as done.Nov 25 2020, 7:37 AM
ZarkoCA added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6984

You're right, there was an clang patch that already handled this.

ZarkoCA updated this revision to Diff 307720.Nov 25 2020, 3:11 PM
ZarkoCA marked an inline comment as done.

Removed passing vector parameters to the stack portion of patch, fixed test case and added test cases for errors.

ZarkoCA edited the summary of this revision. (Show Details)Nov 25 2020, 3:12 PM
sfertile accepted this revision.Nov 25 2020, 4:52 PM

1 comment, but otherwise LGTM.

Sorry Zarko, I must have deleted my comment before posting last night I've added it back now.

llvm/test/CodeGen/PowerPC/aix-cc-ext-vec-abi.ll
53

I think we should use a variable here to make sure the register used in the toc entry access is the same as used in the vector load:

lwz [[SCRATCH1:[0-9]+]], L..C0(2)
lxvw4x 1, 0, [[SCRATCH1]]
ZarkoCA updated this revision to Diff 307866.Nov 26 2020, 7:26 AM

Updated regex in test cases to check register correct usage.

ZarkoCA marked an inline comment as done.Nov 26 2020, 7:26 AM