Page MenuHomePhabricator

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

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.

The vector registers are also added and enabled for AIX.

This patch depends on https://reviews.llvm.org/D88676 which enables the frame lowering and https://reviews.llvm.org/D89684 which enables the mvecnvol and mnovecnvol options.

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.Mon, Sep 28, 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.Mon, Sep 28, 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.Mon, Sep 28, 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.Mon, Oct 5, 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
6933–6934

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

6952

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?

7054

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.Thu, Oct 15, 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.Thu, Oct 15, 2:41 PM
ZarkoCA added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
6933–6934

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

6952

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.

7054

Good catch, thanks.

ZarkoCA updated this revision to Diff 298995.Mon, Oct 19, 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.