Page MenuHomePhabricator

[PPC][AIX] Implement variadic function handling in LowerFormalArguments_AIX
ClosedPublic

Authored by ZarkoCA on Mar 13 2020, 6:49 AM.

Details

Summary

This patch adds support for handling of variadic functions for AIX. This includes ensuring that use and consume correct type of va_list (char *va_list) for AIX.

This patch is dependent on https://reviews.llvm.org/D76360 to emit the correct IR in 32BIT AIX.

Diff Detail

Event Timeline

ZarkoCA created this revision.Mar 13 2020, 6:49 AM
ZarkoCA updated this revision to Diff 250213.Mar 13 2020, 8:09 AM

Clang formatted the patch and fixed typo in testcase.

As a first step, I suggest we break the clang changes and the LLVM changes into 2 separate patches.

ZarkoCA updated this revision to Diff 250749.Mar 17 2020, 6:35 AM
  • Removed 64BIT support varargs.
  • Added a fatal error for 64bit varargs on AIX.
  • Removed 64bit testcases
ZarkoCA retitled this revision from [PPC][AIX] Implement variadic function handling in LowerFormalArguments_AIX. to [PPC][AIX] Implement variadic function handling in LowerFormalArguments_AIX in 32B-bit mode..Mar 17 2020, 6:36 AM
ZarkoCA retitled this revision from [PPC][AIX] Implement variadic function handling in LowerFormalArguments_AIX in 32B-bit mode. to [PPC][AIX] Implement variadic function handling in LowerFormalArguments_AIX in 32-bit mode..
ZarkoCA updated this revision to Diff 250772.Mar 17 2020, 7:45 AM

Removed unrelated changes in aix-cc-abi.ll

ZarkoCA planned changes to this revision.Mar 17 2020, 12:27 PM

I will split this patch up in two. One for changes in clang specific for 32BIT AIX varargs, and a second for changes in llc for 32/64BIT varargs.

jasonliu added inline comments.
clang/lib/Basic/Targets/PPC.h
373 ↗(On Diff #250213)

nit: add '.' to the comment while we are at it.

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

Would it be better if we remove the "else if" and make it

if (VA.isMemloc()){
...
continue;
}

Since this matches the last if statement, and also the style in LowerCall_AIX.

7206

We could remove this definition if we have one defined outside.

7237

The style in this file is "static const" instead of "const static".

7245

This only gets used by 32bit-SVR4 target, I don't think we need to set it.

7250

start a new line before the comment.

7256

Just curious, in what scenario would we already have the GPR stored in a virtual register before we store that GPR on that stack?

llvm/test/CodeGen/PowerPC/aix-cc-abi-va_args-32.ll
9 ↗(On Diff #250772)

I'm not seeing any attribute here, so I'm assuming every "#number" and comments about "Function Attrs" in the test could be removed.

ZarkoCA updated this revision to Diff 251065.Mar 18 2020, 6:42 AM
ZarkoCA retitled this revision from [PPC][AIX] Implement variadic function handling in LowerFormalArguments_AIX in 32-bit mode. to [PPC][AIX] Implement variadic function handling in LowerFormalArguments_AIX.
ZarkoCA edited the summary of this revision. (Show Details)
ZarkoCA added a reviewer: jasonliu.

Addressed comments by Jason.
Re-added 64BIT changes for LLC.
Split clang changes and made a patch specific with them https://reviews.llvm.org/D76360

Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2020, 6:42 AM
ZarkoCA marked 8 inline comments as done.Mar 18 2020, 6:59 AM
ZarkoCA added inline comments.
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7256

I don't think we need to include that, it was a remnant from an earlier prototype I had. Thanks for pointing it out.

ZarkoCA updated this revision to Diff 251111.Mar 18 2020, 9:26 AM
ZarkoCA marked an inline comment as done.

Fixed testcase breakages.

jasonliu added inline comments.Mar 18 2020, 11:54 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7246

This two array gets used to in CC_AIX and here. It would be great if we only have one set of copy.

7247

Could we add an assert to make sure "array_lengthof(GPR_32) == array_lengthof(GPR_64)"
nit:
unsigned const -> const unsigned

7257

Could we do

const unsigned VReg = IsPPC64 ? MF.addLiveIn(GPR_64[GPRIndex], &PPC::G8RCRegClass)
                              : MF.addLiveIn(GPR_32[GPRIndex], &PPC::GPRCRegClass);
llvm/test/CodeGen/PowerPC/aix-cc-abi-va_args-32.ll
27 ↗(On Diff #251111)

These tests used for loops to call va_arg mutliple times.
Maybe it's just me, but I found the test cases are much harder to read.
I would suggest maybe just unroll the for loop and generate as much as the va_arg call that you need?

For the cases below, I suppose you would want to call va_arg 9 times, and 2 times respectfully. Maybe it's not that bad. If the tests gets too long, you could consider split the tests into separate files.

33 ↗(On Diff #251111)

a is 1, so we would only call va_arg once, despite we have 9 vaarg argument. Is this the intention of the test?

162 ↗(On Diff #251111)

We only have 2 va_arg arguments here, but we called va_arg 8 times? I think that's undefined behavior.

jasonliu added inline comments.Mar 18 2020, 12:41 PM
llvm/test/CodeGen/PowerPC/aix-cc-abi-va_args-32.ll
1 ↗(On Diff #251111)

Do we need '-O2' specified here and below?

15 ↗(On Diff #251111)

These variables didn't get used anywhere.

jasonliu added inline comments.Mar 18 2020, 1:13 PM
llvm/test/CodeGen/PowerPC/aix-cc-abi-va_args-32.ll
7 ↗(On Diff #251111)

A general comment about how to test va_arg.
I think the two test cases we have here are not enough.
On top of my head I would probably add in getting double as va_arg argument, uses of va_copy and so on.
It's probably worth checking how other targets test va_arg. Grep "va_start" in llvm/test/CodeGen would give you a lot of hit.

jasonliu added inline comments.Mar 18 2020, 1:26 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7185

Do we need && !VA.needsCustom() here?

ZarkoCA updated this revision to Diff 251408.Mar 19 2020, 10:02 AM
ZarkoCA marked 11 inline comments as done.

Addressed code comments, working on adding and fixing tests for further clarity.

ZarkoCA added inline comments.Mar 19 2020, 11:02 AM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7185

Yes, it's better if I add it, thanks.

7246

I agree, but the array is local to CC_AIX as far as I understand and there is not a good way to access it from here without having to modify CC_AIX. I think it would be a good to do it at a later point when it's not undergoing so much change already.

llvm/test/CodeGen/PowerPC/aix-cc-abi-va_args-32.ll
1 ↗(On Diff #251111)

It makes the IR and assembly much cleaner and easier to read, imo.

7 ↗(On Diff #251111)

Yes, I can clean up the tests and add more. Agreed we need more and I can simplify them so it's clear what's being tested.

15 ↗(On Diff #251111)

Removed, but as I said earlier, I'll also do a rework of the tests.

27 ↗(On Diff #251111)

Yes, see reply above, I will rework the tests.

33 ↗(On Diff #251111)

a = 1 doesn't mean we call va_arg once. This is the IR for the function definition that I got using this c code (modified for this example):

a = 1;
int va_arg1(int a,...) {
...
}

The callee doesn't know how many vararg arguments will be called, just does the setup for the variadic parameter (...) in the IR.

162 ↗(On Diff #251111)

Seem my reply above. That said, there is some confusion with respect to tests and they'll be reworked.

ZarkoCA updated this revision to Diff 252147.Mar 23 2020, 2:56 PM

Simplified testcases.
Added testcases for variadic arguments being passed directly to the stack when all registers are used.

jasonliu added inline comments.Mar 24 2020, 1:01 PM
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
7242

I'd prefer us to keep the original naming convention(GPR_32) for these two arrays, so that it's easy for us to common them up later in an NFC patch.

llvm/test/CodeGen/PowerPC/aix32-cc-abi-vaarg.ll
18

I think it would be more clear if we actually have another va_list to copy instead of just va_copying itself. The same applies to all the places we call va_copy.
Also it's not very clear to me which part in the check result actually tells me about the effect of the va_copy though.

98

llvm.stackprotector could be removed?

ZarkoCA marked 2 inline comments as done.Mar 30 2020, 8:03 PM
ZarkoCA added inline comments.
llvm/test/CodeGen/PowerPC/aix32-cc-abi-vaarg.ll
18

Those are both good points. I'm rewriting the test and adding comments to the IR/assembly to make it clearer.

ZarkoCA marked an inline comment as done.Mar 30 2020, 8:05 PM
ZarkoCA updated this revision to Diff 253774.Mar 30 2020, 8:20 PM

Rebased to include byval changes.

Fixed name of array.

Fixed up test case as per comment.

ZarkoCA planned changes to this revision.Mar 31 2020, 7:15 AM

Fixing test case issues.

ZarkoCA updated this revision to Diff 254193.Apr 1 2020, 7:30 AM

Fixed test cases that were breaking.

jasonliu added inline comments.Apr 1 2020, 8:03 AM
llvm/test/CodeGen/PowerPC/aix32-cc-abi-vaarg.ll
9

minor nit: I don't think these target datalayout and triple are necessary.

ZarkoCA updated this revision to Diff 254243.Apr 1 2020, 10:16 AM
ZarkoCA marked an inline comment as done.

Removed data layout and target triple info from test output.

ZarkoCA updated this revision to Diff 255321.Apr 6 2020, 7:14 AM

Rebase and ping.

jasonliu accepted this revision.Apr 6 2020, 1:17 PM

Not sure if you want to wait for the feedback from @sfertile and @cebowleratibm , but I'm good with the current revision.

This revision is now accepted and ready to land.Apr 6 2020, 1:17 PM
This revision was automatically updated to reflect the committed changes.