Page MenuHomePhabricator

[PPC][AIX] Emit correct Vaarg for 32BIT-AIX in clang
AbandonedPublic

Authored by ZarkoCA on Mar 18 2020, 6:35 AM.

Details

Summary

This patch contains only the FE changes previously found in https://reviews.llvm.org/D76130.
Currently all PPC32-bit targets take the PPC32SVRABI path, this patch branches within that path to consume the va_list emitted for AIX and emit the correct VAArg.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Herald added a project: Restricted Project. · View Herald TranscriptMar 18 2020, 6:35 AM
sfertile added inline comments.Mar 18 2020, 7:54 AM
clang/lib/Basic/Targets/PPC.h
373 ↗(On Diff #251061)

Minor nit: target --> targets.

clang/lib/CodeGen/TargetInfo.cpp
4207–4208

This name and comment is misleading, the class is used for both SVR4 and Darwin, and after this patch AIX. We need to fix the name comment to reflect that.

hubert.reinterpretcast added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
4309–4312

No need for two spaces. Add comma after "Currently".

clang/test/CodeGen/aix-vararg.c
5

Can we use built-in types and functions in place of a header inclusion?

jasonliu added inline comments.Mar 18 2020, 8:13 AM
clang/test/CodeGen/aix-vararg.c
11

As part of a "va_..." family, do we also want to test va_copy?

ZarkoCA marked 8 inline comments as done.Mar 19 2020, 9:57 AM
ZarkoCA added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
4207–4208

Does this wording of the comment work?

4309–4312

Thanks, I've found it hard to shake the habits my Grade 8 typing teacher taught me.

clang/test/CodeGen/aix-vararg.c
5

I'm worried about legal/copyright issues with using contents from AIX system headers to LLVM testcases. But I can do that for sure once I understand what I am allowed to do.

11

Good suggestion, added.

ZarkoCA updated this revision to Diff 251407.Mar 19 2020, 9:59 AM
ZarkoCA marked 3 inline comments as done.

Changed comments per suggestions.
Added va_copy in test case.

clang/lib/CodeGen/TargetInfo.cpp
4208

I would suggest using an "Oxford comma" before the "and". Also, colon instead of comma before "used".

clang/test/CodeGen/aix-vararg.c
5

I just mean:
__builtin_va_list
__builtin_va_start
__builtin_va_copy
__builtin_va_arg
__builtin_va_end

sfertile added inline comments.Mar 23 2020, 10:10 AM
clang/lib/CodeGen/TargetInfo.cpp
4207–4208

Missed updating the class name. Suggested: PPC32_SVR4_ABIInfo --> PowerPC32ABIInfo.

ZarkoCA marked 6 inline comments as done.Mar 24 2020, 7:57 AM
ZarkoCA added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
4207–4208

Sorry, missed updating the class name the first time.

clang/test/CodeGen/aix-vararg.c
5

Thanks, I wasn't clear. Done as that now.

ZarkoCA updated this revision to Diff 252325.Mar 24 2020, 8:00 AM
ZarkoCA marked 2 inline comments as done.

Renamed PPC32_SVR4ABIInfo class to PPC32ABIInfo.
Fixed test case to use builtins.
Changed comment.

jasonliu added inline comments.Mar 24 2020, 1:16 PM
clang/lib/CodeGen/TargetInfo.cpp
4255

So for AIX we are taking PowerPC32ABIInfo right now. And we know EmitVAArg of this target does the right thing on AIX after the change.
But for other functions, for example, getParamTypeAlignment, initDwarfEHRegSizeTable... Are we sure it's doing the right thing on AIX?
If we are not sure, is there anything we want to do (etc, put a comment on where it gets used or at the function definition)? Or are we fine to just leave it as it is and have a TODO in our head?

sfertile added inline comments.Mar 24 2020, 3:14 PM
clang/lib/CodeGen/TargetInfo.cpp
4255

Looking at the values in initDwarfEHRegSizeTable it has the right sizes for all the registers. Even though the OS is different the underlying hardware is the same. I'm not sure it's something that makes sense to support for AIX though, in which case I think its valid to return true to indicate its not supported.

getParamTypeAlignment is used only in the context of the alignment for vaarg, we should make sure its correct for AIX since supporting vaarg is the scope of this patch.

4308–4309

Make sure to address the formatting here.

jasonliu added inline comments.Mar 26 2020, 10:05 AM
clang/lib/CodeGen/TargetInfo.cpp
4255

In that case, is it better if we have lit test to actually exercise the path in getParamTypeAlignment to show that we actually confirmed the behavior is correct for AIX? And if it is some path we do not care for now(ComplexType? VectorType?), then we would want TODOs on them to show we need further investigation later.

ZarkoCA updated this revision to Diff 253127.Mar 27 2020, 8:39 AM

Created PPCAIX32TargetCodeGenInfo class so that initDwarfEHRegSizeTable now returns true on AIX and added a test.

Fixed formatting.

ZarkoCA marked 2 inline comments as done.Mar 27 2020, 8:59 AM
ZarkoCA added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
4255

But for other functions, for example, getParamTypeAlignment, initDwarfEHRegSizeTable... Are we sure it's doing the right thing on AIX?

The AIX stdarg headers show that alignment for the va_list is dependent on mode, so 4 on 32 and 8 on 64-bit. getParamAlignment looks like it aligns 4 for non-complex non-struct args.

If we are not sure, is there anything we want to do (etc, put a comment on where it gets used or at the function definition)? Or are we fine to just leave it as it is and have a TODO in our head?

We already took this path before this patch and I'm correcting some of the behaviour so it's correct for varargs on AIX. I think that we will need to address this depending on when/if we add an AIXABIInfo class. I added a TODO on line 4247 for just that so I think that implies we will investigate everything required for the correct implementation of the whatever remains after the vaarg handling.

jasonliu added inline comments.Mar 30 2020, 7:54 AM
clang/lib/CodeGen/TargetInfo.cpp
10378

Does AIX have soft Float? If not, do we want to always pass in 'false'?

clang/test/CodeGen/aix-vararg.c
5

Any reason we don't use __builtin_va... any more?

ZarkoCA updated this revision to Diff 253776.Mar 30 2020, 8:37 PM

Fixed test cases to use builtins again, set no soft float abi for AIX.

ZarkoCA marked 4 inline comments as done.Mar 30 2020, 8:40 PM
ZarkoCA added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
10378

Thanks, missed changing this. I set it to hard.

clang/test/CodeGen/aix-vararg.c
5

My mistake, I somehow included the old version of the test in the diff.

jasonliu added inline comments.Apr 1 2020, 7:43 AM
clang/lib/CodeGen/TargetInfo.cpp
10378

I don't think CodeGenOpts.FloatABI == "hard" is what we want though. Currently it means if CodeGenOpts.FloatABI is really "hard", then it will pass in true for SoftFloatABI to indicate we are soft float ABI.

ZarkoCA marked 4 inline comments as done.Apr 1 2020, 8:24 AM
ZarkoCA added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
10378

You're right. I wanted to keep the symmetry but that's not the correct thing to do.

ZarkoCA updated this revision to Diff 254214.Apr 1 2020, 8:26 AM
ZarkoCA marked an inline comment as done.

Set isSoftFloat to return false for AIX.

Xiangling_L added inline comments.
clang/test/CodeGen/aix-vararg.c
16

#0, #1[the last three lines] are redundant, could you clean them up?

Xiangling_L added inline comments.Apr 6 2020, 5:12 PM
clang/lib/CodeGen/TargetInfo.cpp
4254

I have a question here. AIX32 falls into PPC32 target, so why we don't inherit from PPC32TargetCodeGenInfo instead?

4259

Is getDwarfEHStackPointer necessary to be correct for vararg of AIX to work[I guess possibly not]? If not, should it fall into Dwarf related patch rather than in this one? BTW, if your PPCAIX32TargetCodeGenInfo inherits from PPC32TargetCodeGenInfo instead as I mentioned above, then it would be naturally correct.

4538

As simple as this function is, does it make sense to move the body of return true into the class definition?

sfertile added inline comments.Apr 9 2020, 7:01 AM
clang/lib/CodeGen/TargetInfo.cpp
4254

Do we need a separate AIX specific class? We are implementing 2 functions, 1 of which is the same implementation as its PPC32TargetCodeGenInfo counterpart. If we have access to the triple, we can return true when the OS is AIX in PPC32TargetCodeGenInfo::initDwarfEHRegSizeTable. With the implementations being nearly identical (and after enabling DwarfEHRegSizeTable they will be identical) I think we are better to not add a new class if we can avoid it.

Xiangling_L added inline comments.Apr 9 2020, 7:19 AM
clang/lib/CodeGen/TargetInfo.cpp
4254

Not adding a new class makes sense to me if we are sure that DwarfEHRegSizeTable will be identical/viable for AIX.

ZarkoCA marked 8 inline comments as done.Apr 9 2020, 11:50 AM
ZarkoCA added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
4254

I had an offline conversation with @sfertile, verified that the existing initDwarfEHRegSizeTable() function can work for AIX with minor changes.

At first, I thought it may be useuful to create a stub class for for AIX at least. But I was able to access the AIX triple info through getABIInfo() and we can keep the one PPC32 class and diverge for AIX within the function when required.

4259

It's not related to vaarg but the implementation is pretty simple so we might as well do it since we are here. I also added a testcase with this enabled.

ZarkoCA updated this revision to Diff 256351.Apr 9 2020, 12:09 PM
ZarkoCA marked 2 inline comments as done.

Rebased on https://reviews.llvm.org/D73290 and this patch now depends on it.
Removed PPCAIX32TargetCodegenClass from previous diff.
Corrected behaviour PPC32TargetCodeGenInfo::initDwarfEHRegSizeTable() for 32BIT AIX, and added the expected output to the test case which has also been renamed to reflect that.

A couple minor comments, but patch is almost ready otherwise.

clang/lib/CodeGen/TargetInfo.cpp
4281

Pedantic nit: Can we emit a fatal error if -msvr4-struct-return is specified on AIX. Then we can add a lit test which checks for the error, which will change once we verify the options behavior on AIX and enable it.

Note: FWIW both Zarko and I have verified the option is accepted and work as expected by passing in r3/r4, I'm just not sure if any padding is handled the same way on AIX as described below.

4521

Minor nit:

Can we restructure like this:

 // 109: vrsave
// 110: vscr
AssignToArrayRange(Builder, Address, Four8, 109, 110);

  // AIX does not have SPE registers so we don't set 111 - 113.
  if (getABIInfo().getTarget().getTriple().isOSAIX()) 
  return false;

// 111: spe_acc
  // 112: spefscr
  // 113: sfp
  AssignToArrayRange(Builder, Address, Four8, 111, 113);

return false;
clang/test/CodeGen/ppc64-aix32-dwarf.c
1 ↗(On Diff #256351)

Minor nit: How about naming the test file ppc-dwarf.c

ZarkoCA updated this revision to Diff 256398.EditedApr 9 2020, 1:54 PM
ZarkoCA edited the summary of this revision. (Show Details)

Addressed comments
-added error for -msvr4-struct-return on AIX and modified appropriate test case
-changed code structure as per comment
-renamed test file

ZarkoCA marked 2 inline comments as done.Apr 9 2020, 1:58 PM
sfertile accepted this revision.Apr 13 2020, 7:22 AM

LGTM.

This revision is now accepted and ready to land.Apr 13 2020, 7:22 AM
jasonliu added inline comments.Apr 13 2020, 8:30 AM
clang/test/CodeGen/ppc32-struct-return.c
53

If certain front end option is not supported on certain target, I think it makes more sense to have a standard diagnostic in the driver component, instead of "crash" in the backend.
i.e. What if we specify this option on a Windows machine? Maybe we should pursue the same behavior.

sfertile added inline comments.Apr 14 2020, 7:15 AM
clang/test/CodeGen/ppc32-struct-return.c
53

Its not that we don't intend to support the option on AIX, but that support for the option takes further verification on AIX. Since there is a difference how AIX justifies subregister sized values in its argument passing, we can't just assume that this option will pack the return values the same way on AIX and Linux.

The focus of this patch was originally to enable and verify the proper IR generation of va-arg/va-lis/va-start, however the scope of the patch has kept growing. If there are codegen differences in packing the return register with the svr4-return option enabled it will grow this patch once again. The fatal error lets us limit the scope of this patch, while not silently emiting bad codegen if there is a difference in how gcc initializes the return registers. If during verification we decide we don't want to support the option on AIX, then adopting a standard diagnostic in the driver component becomes the appropriate behavior.

jasonliu added inline comments.Apr 14 2020, 7:53 AM
clang/test/CodeGen/ppc32-struct-return.c
53

It wasn't clear for me from the code that this is not a permanent thing. In that case, does it make sense to leave a TODO here and say we need to re-evaluate this in the future to decide if we want to support it or not on AIX?
On another note, I'm not sure if this is really needed on AIX target though. But I guess we could discuss it down the road.

jasonliu added inline comments.Apr 14 2020, 7:55 AM
clang/test/CodeGen/ppc32-struct-return.c
53

Just to avoid ambiguity, I meant I'm not sure if we really need this *option* on AIX.

ZarkoCA marked an inline comment as done.Apr 15 2020, 1:48 PM
ZarkoCA added inline comments.
clang/test/CodeGen/ppc32-struct-return.c
53

The focus of this patch was originally to enable and verify the proper IR generation of va-arg/va-lis/va-start, however the scope of the patch has kept growing. If there are codegen differences in packing the return register with the svr4-return option enabled it will grow this patch once again. The fatal error lets us limit the scope of this patch, while not silently emiting bad codegen if there is a difference in how gcc initializes the return registers. If during verification we decide we don't want to support the option on AIX, then adopting a standard diagnostic in the driver component becomes the appropriate behavior.

@sfertile basically articulated my reasoning quite well here. I just want to add, if verified to work on AIX, then we can simply remove the error in that one place and have the option working.

GCC on AIX has those options and they work in the same way as described in https://reviews.llvm.org/D73290. I think implementing a compatible GCC option in a fairly straightforward way doesn't hurt us.

I agree with your point made earlier, I will add a TODO to make it clearer that we need to completely verify the behaviour.

ZarkoCA updated this revision to Diff 258110.Apr 16 2020, 11:28 AM

Added a TODO to remove the error for msvr4-struct-return on AIX when we verify it works as expected.

ZarkoCA marked 4 inline comments as done.Apr 16 2020, 11:29 AM
ZarkoCA updated this revision to Diff 261837.May 4 2020, 8:59 AM
ZarkoCA edited the summary of this revision. (Show Details)

Rebased patch to include latest changes in trunk. Removed that it depended on https://reviews.llvm.org/D73290 in the summary, as that patch has been landed.

ZarkoCA abandoned this revision.May 5 2020, 7:08 AM

This work is included in https://reviews.llvm.org/D79035. Abandoning this revision.