Page MenuHomePhabricator

[clang][AIX] Implement ABIInfo and TargetCodeGenInfo for AIX
ClosedPublic

Authored by jasonliu on Apr 28 2020, 1:06 PM.

Details

Summary

Created AIXABIInfo and AIXTargetCodeGenInfo for AIX ABI. Notice that we could consider common up PowerPC related TargetCodeGenInfo later, as they are almost the same except the constructor.

Some investigation and FAQ on why we created AIXABIInfo:

Use or derive PPC32/PPC64 ABIInfo for AIX:
There are a lot of subtle differences between PPC32 and PPC64 variation. For AIX we do not have a huge differences between 32 bit and 64 bit for the ABI rules. Which means if we decide to use PPC32 for 32 bit on AIX and PPC64 for 64 bit on AIX, we will need to add in a lot of target check to make them symmetric. The code flow will be really hard to follow and verify for every target that is trying to use them. And it’s easy for us to take a lot of unwanted change that really is meant for SVR targets.
Use one ABIInfo class for both 32 bit and 64 bit on AIX will make the code much clear and easier to follow through. And that was one of the reason for us to create LowerCall_AIX/LowerFormalArgument_AIX instead of just rename and use the existing SVR4/Darwin one.

Derive from DefaultABIInfo:
We need to override most of the methods in DefaultABIInfo anyway.

Derive from SwiftABIInfo:
I don't think we want to claim Swift support on AIX right now.

Diff Detail

Event Timeline

jasonliu created this revision.Apr 28 2020, 1:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2020, 1:06 PM
jasonliu edited the summary of this revision. (Show Details)Apr 28 2020, 2:09 PM
Xiangling_L added inline comments.May 4 2020, 2:39 PM
clang/lib/CodeGen/TargetInfo.cpp
4254

Minor comment about comment style:
Though I noticed that "AFAIK all ABIs use the same encoding." is from original code, could we adjust it to something like "All PPC ABIs use the same encoding."

4269

s/64-76/64-67?

4392

This method uses the ABI alignment of the given aggregate type which I think is not ideal due to our AIX special alignment rule. We need to use preferred alignment in this case.
Btw also I think it's not necessary for you to rebase your patch on the power alignment patch, I can refresh the testcase when I am dealing with that one.

4411

Same comment like above.

clang/test/CodeGen/aix-vaargs.c
4

Consistent with other testcases to use AIX32/AIX64?

clang/test/CodeGen/ppc32-and-aix-struct-return.c
8

Do you mean to check AIX or SVR4?

clang/test/CodeGen/ppc32-dwarf.c
3

Minor comment:
Would PPC32SVR4 compared to PPC32 make the checking content clearer since PPC32 actually includes AIX target?

clang/test/CodeGen/ppc64-dwarf.c
2

Same comment as above.
s/PPC64/PPC64SVR4?

jasonliu marked 2 inline comments as done.May 4 2020, 4:14 PM
jasonliu added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
4392

As it is right now in master, there is no difference between natural alignment and preferred alignment for AIX. The tentative direction is to use preferred alignment to record the actual alignment on AIX, but it is not finalized yet. I would rather leave this part of the work for the patch that's going to implement the power alignment rule for AIX.

clang/test/CodeGen/aix-vaargs.c
4

I chose AIX-M32/AIX-M64 mainly because the length is the same as AIX-COM so we don't need to worry about aligning the space. I would prefer to keep it that.

jasonliu marked 2 inline comments as done.May 4 2020, 4:21 PM
jasonliu added inline comments.
clang/test/CodeGen/ppc32-dwarf.c
3

Technically, it's PPC32 target except AIX (not restrict to SVR4). So PPC32SVR4 is not that accurate either.

clang/test/CodeGen/ppc64-dwarf.c
2

Same above, and for PPC64 we have Darwin that's actually not SVR4.

ZarkoCA added inline comments.May 5 2020, 5:57 AM
clang/lib/CodeGen/TargetInfo.cpp
4443

Is there a reason why Indirect is set to false instead of querying for it using classifyArgumentType(Ty).isIndirect()?

4732

Has this option been verified to work correctly on AIX? In https://reviews.llvm.org/D76360 we added a defensive error because we weren't sure whether padding was handled correctly as described in the code.

4745–4746

Should be changed to /*Is64BIT*/? Or you can add spaces to`/*IsAIX*/` so that it's consistent.

5298

Remove extra space from /* Is64Bit*/

10633

same as above about removing extra spaces.

10644

spaces

clang/test/CodeGen/aix-return.c
4

I think it would be simpler to just use AIX instead of AIX-COM.

clang/test/CodeGen/aix-struct-arg.c
4

Same here, I think it be simpler to use either CHECK or AIX. My preference is to use CHECK since other targets aren't being tested here but I would leave it up to you.

clang/test/CodeGen/aix-vaargs.c
4

I agree with Xiangling, it would be better to be consistent with the other testcases. To get things to line up properly you can use AIX32/AIX64 and CHECK instead of AIX-COM.

Xiangling_L added inline comments.May 5 2020, 6:52 AM
clang/lib/CodeGen/TargetInfo.cpp
4392

getNaturalAlignIndirect uses ABI align which is for sure not correct on AIX target for Aggregate types. What we want is the actual alignment here.

So I am not sure if we want to take getNaturalAlignIndirect for granted even if we know it's not correct semantically.

Xiangling_L added inline comments.May 5 2020, 7:39 AM
clang/lib/CodeGen/TargetInfo.cpp
4392

Comment update:
For the power alignment patch, after some investigation, to test IR alignment value for struct as argument and return type, it should base on this ABI patch.

So I agree that we can rely on power alignment patch later to update this part.

jasonliu updated this revision to Diff 262250.May 5 2020, 4:01 PM
jasonliu marked 12 inline comments as done.

Rebase and address comments.

clang/lib/CodeGen/TargetInfo.cpp
4443

For how we handle Vaarg in backend, we just expect everything on register(no ByVal) even when it's a structure. So the indirect should always be false unless there is change in backend implementation.

4732

Thanks. I think we should disable this option in this patch.

clang/test/CodeGen/ppc32-and-aix-struct-return.c
8

This file is actually from llvm-project/clang/test/CodeGen/ppc32-struct-return.c
What I did is only adding the check for AIX triple. And apparently, the default behavior of powerpc-unknown-linux triple is CHECK-AIX.

jasonliu updated this revision to Diff 262252.May 5 2020, 4:05 PM
ZarkoCA added inline comments.May 7 2020, 9:13 AM
clang/lib/CodeGen/TargetInfo.cpp
4301

From what I've seen, the tfhar, tfiar and texasr are used by the Power8 CPU, which means that there is potential for them to be used on 64BIT AIX. I don't have access to a Power8 AIX machine to test and confirm this however. If no one else can confirm, maybe it's good to leave a TODO here to check?

clang/lib/Driver/ToolChains/Clang.cpp
4541 ↗(On Diff #262252)

-maix-struct-return behaves as expected on AIX (ie. it has no change from default behaviour) but I agree, to me it makes disable both if we are not sure about one of them. However, I think it would be good to add a TODO to enable this once it's verified on AIX.

clang/test/CodeGen/ppc32-and-aix-struct-return.c
55

sorry, is it possible to lineup the define .. with the line below, for the entire testcase?

jasonliu updated this revision to Diff 262869.May 8 2020, 7:17 AM

Address comments.
Add in over aligned structure and structure containing vector type handling for argument on AIX.

jasonliu marked 3 inline comments as done.May 8 2020, 7:17 AM
jasonliu updated this revision to Diff 262878.May 8 2020, 8:05 AM
ZarkoCA added inline comments.May 8 2020, 11:54 AM
clang/lib/CodeGen/TargetInfo.cpp
5304–5305

Missed to point out the extra space in /* Is64Bit*/ previously.

clang/lib/Driver/ToolChains/Clang.cpp
4541 ↗(On Diff #262878)

s/enable/enabling/

Xiangling_L added inline comments.May 8 2020, 2:00 PM
clang/lib/CodeGen/TargetInfo.cpp
1589–1590

Also update the comment?

4409

As in doc says The transparent_union type attribute:
float _Complex, double _Complex or vector types can be members of a transparent union, but they cannot be the first member. That means the first field still could be something like Integral Complex etc., which falls into the category isAnyComplexType.

So I guess Ty = useFirstFieldIfTransparentUnion(Ty); should be at the first line.

4418

If we want to be consistent with other part of alignment implementation, getContext().getTypeAlignInChars(Ty) gives ABIAlign. I would suggest we name ABIAlign here to something like CCAlign or ArgumentAlign.

4732

I noticed that in patch https://reviews.llvm.org/D76360, Zarko added a check to emit an error for using this option within cc1. But in your patch, this option only emit error when invoked by the driver. Does that mean we are pretty sure this option is doing what we want on AIX?

jasonliu updated this revision to Diff 262958.May 8 2020, 2:16 PM
jasonliu marked 2 inline comments as done.

Address Zarko's comment.

jasonliu updated this revision to Diff 262964.May 8 2020, 2:37 PM
jasonliu marked 5 inline comments as done.

Address Xiangling's comment.

clang/lib/CodeGen/TargetInfo.cpp
1589–1590

I think the comment is fine. It's still SSE vector type even if we changed the query to SIMD. SSE is for x86 and altivec is for Power, but they are both SIMD vectors.

4732

Are you able to set this CodeGen option when it is disabled in the Frontend/CompilerInvocation.cpp?

jasonliu marked an inline comment as done.May 8 2020, 2:41 PM
Xiangling_L added inline comments.May 12 2020, 7:17 AM
clang/lib/CodeGen/TargetInfo.cpp
4732

I would say if you disable it in this function PPC32TargetCodeGenInfo::isStructReturnInRegABI, neither driver and FE can invoke it. However, in your patch, the option is disabled only in the driver, that means you can still invoke it with clang_cc1.

clang/test/CodeGen/aix-vector.c
12

Please remove the extra blank line

jasonliu updated this revision to Diff 263502.May 12 2020, 1:09 PM

Add error report in clang_cc1 for unsupported option on AIX.

Xiangling_L added inline comments.May 13 2020, 7:59 AM
clang/lib/Frontend/CompilerInvocation.cpp
1301

Since we disable them in FE, should we remove the one in driver?

clang/test/Frontend/aix-unsupported.c
11

One thing I am not so sure about is that for these two options -maix-struct-return, -msvr4-struct-return, do we need to update the ClangCommandLineReference.rst since we emit diags unsupported option '-maix-struct-return' for target 'powerpc-unknown-aix'

jasonliu marked an inline comment as done.May 13 2020, 9:15 AM
jasonliu added inline comments.
clang/test/Frontend/aix-unsupported.c
11

I think the error message is clear enough in itself what user should expect. I don't think it's necessary to add to the doc.

jasonliu updated this revision to Diff 263864.May 13 2020, 2:57 PM

Remove driver's error.

jasonliu marked an inline comment as done.May 13 2020, 2:58 PM

I'm ok with this now. I will let @Xiangling_L approve if she's ok with it since she had the last few comments.

Xiangling_L accepted this revision.May 14 2020, 10:37 AM

LGTM with a minor comment:
You may want to add a TODO or FIXME at clang/lib/CodeGen/TargetInfo.cpp: 4496 and clang/lib/CodeGen/TargetInfo.cpp:4418 like:

//FIXME:
Correct the aggregate type alignment when it contains a double/long double as its first member.
This revision is now accepted and ready to land.May 14 2020, 10:37 AM
This revision was automatically updated to reflect the committed changes.