Page MenuHomePhabricator

[AIX][Frontend] C++ ABI customizations for AIX boilerplate
ClosedPublic

Authored by Xiangling_L on Feb 4 2020, 6:07 PM.

Details

Summary

This PR enables XL C++ ABI in frontend AST to IR codegen. And it is driven by static init work. The current kind in Clang by default is Generic Itanium, which has different behavior on static init with XLClang on AIX.

More context where the static init will be going refers to the patch: D74166.

Diff Detail

Event Timeline

Xiangling_L created this revision.Feb 4 2020, 6:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 4 2020, 6:07 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Xiangling_L edited the summary of this revision. (Show Details)Feb 6 2020, 12:02 PM
Xiangling_L edited the summary of this revision. (Show Details)
daltenty added inline comments.Feb 11 2020, 7:24 AM
clang/include/clang/Basic/TargetCXXABI.h
116

Why the underscore in the name? This is a bit inconsistent with both the LLVM naming convention here and the name as it appears in other sources.

stevewan added inline comments.
clang/lib/CodeGen/ItaniumCXXABI.cpp
520

The class name here is inconsistent with how our ABI kind was called previously, as David pointed out. Maybe rename the ABI kind XLClang?

Xiangling_L marked 4 inline comments as done.Feb 11 2020, 1:22 PM
Xiangling_L added inline comments.
clang/include/clang/Basic/TargetCXXABI.h
116

There are various AIX ABI. So to distinguish the one we are implementing, we choose XL and Clang as two parts of the abi name.
XL - not g++;
Clang - it's a ABI implemented in Clang;

And also XLClang is misleading because it represents our AIX XL C/C++ compiler itself externally.

clang/lib/CodeGen/ItaniumCXXABI.cpp
520

I understand you concerns, and please see my replies above.

Xiangling_L marked 2 inline comments as done.Feb 11 2020, 1:37 PM
sfertile added inline comments.Feb 11 2020, 1:41 PM
clang/include/clang/Basic/TargetCXXABI.h
116

So do we need the 'Clang' part in the name? For example the ABI below is not Microsoft_Clang. Or is the _Clang differentiating between multiple XL ABIs?

Looks fine, but we need to settle on the name for the ABI. My preference would be "XLC++11", or perhaps "XLCXX11" (I propose the latter because of the common reference CXXABI.)

clang/include/clang/Basic/TargetCXXABI.h
116

I suspect the concern is that "XL" ABI is ambiguious between legacy xlC and xlclang++. The two differ at the C++11 language level so perhaps it makes sense to have "XLC++11"? (and theoretically just "XL" if we ever decide xlC)

clang/lib/CodeGen/ItaniumCXXABI.cpp
4428

Omit "fully". Fix the name XL_Clang when we've settled on a name.

clang/test/CodeGen/static-init.cpp
10

Nit: formatting.

The test is fine but I'd usually write this test even more briefly:
struct S { ~S(); } s;

Xiangling_L marked 4 inline comments as done.

Adjust ABI name to XL;
Simplify the testcase;

sfertile added inline comments.Feb 13 2020, 11:03 AM
clang/lib/CodeGen/ItaniumCXXABI.cpp
520

Here would be a good place to add a comment to indicate that XL has several C++ ABIs, but this represents the one used in 'xlClang++'.

Xiangling_L marked 3 inline comments as done.Feb 13 2020, 1:51 PM
Xiangling_L added inline comments.
clang/lib/CodeGen/ItaniumCXXABI.cpp
520

You mean we have legacy XLC and XLClang++ ABI? But for static init, they have same implementation. So it's not a must to point it out.

And also AFAIK, static init is the only thing we will differ from Generic Itanium ABI in the frontend, so basically it's the only thing we will add in this ABI.

I am okay with either way with a little concern that legacy XLC user may wonder is there any difference of static init implementation between XLC and XLClang++ ABI if we add the comment.

sfertile added inline comments.Feb 13 2020, 5:30 PM
clang/lib/CodeGen/ItaniumCXXABI.cpp
520

Sorry, I had a matching comment on the 'XL' enum, but I must have deleted it accidentally before submitting. I said I agreed with using just 'XL' since there is only one XL C++ ABI implemented in Clang we don't have to worry about differentiating between the 'legacy' XL and the C++11 XL ABIs. If you did want to clarify then adding a comment here would be the only thing I suggest.

Xiangling_L marked 3 inline comments as done.Feb 14 2020, 10:41 AM
Xiangling_L added inline comments.
clang/lib/CodeGen/ItaniumCXXABI.cpp
520

I see. Thank you for your clarification.

Xiangling_L marked an inline comment as done.Feb 18 2020, 6:14 AM

ping.

From my perspective, the only issue holding this up is settling on the name. I'd like to hammer that out and get this committed.

clang/include/clang/Basic/TargetCXXABI.h
116

Another suggestion: "IBMXL" or "IBMXLC++11"

From my perspective, the only issue holding this up is settling on the name. I'd like to hammer that out and get this committed.

It looks everyone agrees on XL so far. As for your other suggestion IBMXL, if your intention is to clarify this ABI is IBM product specific, I think a good practice we can do is to leave a comment when we defined it as Microsoft does.

Patch LGTM as far a formatting/naming/testing etc. C++ specifics is outside my wheelhouse though, so I can't confirm things like the tail padding rules are correct for AIX. Because of that I'm not comfortable being the one to accept the patch.

Patch LGTM as far a formatting/naming/testing etc. C++ specifics is outside my wheelhouse though, so I can't confirm things like the tail padding rules are correct for AIX. Because of that I'm not comfortable being the one to accept the patch.

I checked the IBM xlclang source and can confirm the implementation of getTailPaddingUseRules is consistent with the proposed patch. Sean, does that address your concern?

The only change I would like to see is an update to the "XL" enumerator comment to be more descriptive of what the "XL" ABI is.

clang/include/clang/Basic/TargetCXXABI.h
116

To summarize some off phabricator discussion: we reached consensus on "XL". It was not felt that "IBM" needs to be in the name and that a comment by the enumerator definition is sufficient.

Note that "XL" refers to ABI compatibiliity with the AIX xlclang compiler and not xlC. If we need xlC compatibility that would be yet another ABI to add at another time.

The comment should be updated to provide more background on what the "XL" ABI is.

Xiangling_L marked an inline comment as done.

Update the comment about ABI description;

sfertile accepted this revision.Feb 21 2020, 7:31 AM

LGTM.

This revision is now accepted and ready to land.Feb 21 2020, 7:31 AM

Just kindly reminder that you might want to update the Summary in the commit message to reflect the new name we picked.

Xiangling_L edited the summary of this revision. (Show Details)Feb 24 2020, 6:12 AM
This revision was automatically updated to reflect the committed changes.