Page MenuHomePhabricator

Add AIX Target Info
ClosedPublic

Authored by andusy on Mar 6 2019, 1:44 PM.

Diff Detail

Repository
rC Clang

Event Timeline

andusy created this revision.Mar 6 2019, 1:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2019, 1:44 PM
hubert.reinterpretcast added inline comments.
clang/lib/Basic/Targets/OSTargets.h
626 ↗(On Diff #189580)

Comments should be full sentences (with periods). Please update throughout this patch.

629 ↗(On Diff #189580)

@rsmith suggested that having a -fno-long-long option to disable support for long long would be preferable to having an -maix-no-long-long that just controls the AIX binary-compatibility aspects. Perhaps the FIXME should be updated to mention checking -fno-long-long.

637 ↗(On Diff #189580)

Suggestion for the comment text:
Define _WCHAR_T when it is a fundamental type (i.e., for C++ without -fno-wchar).

641 ↗(On Diff #189580)

D18360 sets _THREAD_SAFE to 1 when -pthread is specified. I believe that to be correct. I believe whether or not -pthread is taken to be the default on the platform is a separate consideration.

651 ↗(On Diff #189580)

Can we have a test for this?

apaprocki added inline comments.Mar 6 2019, 6:55 PM
clang/lib/Basic/Targets/OSTargets.h
626 ↗(On Diff #189580)

I do not think it is realistic to support an AIX target prior to 7.1, so should this unconditionally define up to and including _AIX71 similar to D18360 did (it was written earlier and defined up to _AIX61 unconditionally) and leave a FIXME to determine when to emit _AIX72 or any other later versions?

clang/test/Preprocessor/init.c
6420 ↗(On Diff #189580)

XL on AIX emits #define _LP64 1 in 64-bit mode and #define _ILP32 1 in 32-bit mode in the pre-defined macros. Is that important to capture?

clang/lib/Basic/Targets/OSTargets.h
626 ↗(On Diff #189580)

I agree with your assessment re: older versions of AIX in terms of having a full solution for modern C++. Nevertheless, we can leave room open for hobby builds targeting older versions where the immediate cost is not high. Cross compiling to an older AIX target will probably be entirely possible for C.

clang/test/Preprocessor/init.c
6420 ↗(On Diff #189580)

I think so. The v16.1 XL compiler's xlclang also produces these.

#define __LP64__ 1
#define _LP64 1
#define __ILP32__ 1
#define _ILP32 1
clang/test/Preprocessor/init.c
6420 ↗(On Diff #189580)

It seems GCC on AIX only defines the macros for 64-bit, and not the 32-bit versions. The system headers do not appear to depend on the 32-bit versions. It makes sense to start with the common intersection between the GCC and XL predefined macros first. We can add __LP64__ and _LP64 with this patch. I think we can leave more macros for later.

sfertile added inline comments.Mar 8 2019, 7:59 AM
clang/test/Preprocessor/init.c
6420 ↗(On Diff #189580)

It makes sense to start with the common intersection between the GCC and XL predefined macros first

Agreed

andusy updated this revision to Diff 189890.Mar 8 2019, 11:14 AM
  • Updated comments
  • Added testing for UseZeroLengthBitfieldAlignment
  • Defined _THREAD_SAFE when -pthread is enabled
  • Check for definition of __LP64__ and _LP64 macros
andusy marked 7 inline comments as done.Mar 8 2019, 11:18 AM
andusy added inline comments.
clang/lib/Basic/Targets/OSTargets.h
641 ↗(On Diff #189580)

I've added a test in clang/test/Preprocessor/init.c to check if the macro is defined when -pthread is enabled.

andusy marked 2 inline comments as done.Mar 8 2019, 11:21 AM
andusy added inline comments.
clang/test/CodeGen/arm-aapcs-zerolength-bitfield.c
5 ↗(On Diff #189890)

This file is pending a name change.

clang/lib/Basic/Targets/OSTargets.h
640 ↗(On Diff #189890)

Line is longer than 80 characters. Please split it.

clang/test/CodeGen/arm-aapcs-zerolength-bitfield.c
1 ↗(On Diff #189890)

Given the requirement for arm-registered-target, is the file actually run whenever we intend it to be? Also, a note re: the existing test: This does not seem to be a CodeGen test; indeed, it is effective even with -fsyntax-only. It seems this should be moved to clang/test/Sema.

However, not all of the cases are common anyway. We will later need to post changes that implement AIX's 4-byte storage units for bit-fields. I think we should leave this file alone for the purposes of this patch.

clang/test/Preprocessor/init.c
7027 ↗(On Diff #189890)

Add a check that _LP64 is not defined under the 32-bit mode. Similarly for __LP64__ and __64BIT__. Other targets also check for the presence or absence of _ILP32 and __ILP32__, so we probably should do the same.

andusy updated this revision to Diff 190133.Mar 11 2019, 1:03 PM
  • Fixed comment.
  • Checked that _LP64, __LP64__, and __64BIT__ aren't defined in 32 bit mode.
  • Checked that _ILP32 and __ILP32__ aren't defined in 64 bit mode.
andusy marked 2 inline comments as done.Mar 11 2019, 1:04 PM
hubert.reinterpretcast added a subscriber: WuZhao.

I think all comments have been addressed or otherwise responded to.
@jasonliu: Since this patch is dependent on D58930, I would suggest that you commit it after D58930. A friendly reminder to include the patch attribution for @andusy. @apaprocki, and @WuZhao. Thanks.

This revision is now accepted and ready to land.Mar 11 2019, 3:24 PM
Closed by commit rC356060: Add AIX Target Info (authored by jasonliu, committed by ). · Explain WhyMar 13 2019, 9:03 AM
This revision was automatically updated to reflect the committed changes.

I'm seeing some build bot failures because of the newly added test case: max_align.c.
So I reverted the previous committed change.
Please fix the test case and let me know so that I can help you commit again.

test/Headers/max_align.c
1

We may need to explicitly specify C11. It also seems that we should XFAIL Windows targets.

lebedev.ri added inline comments.
test/Headers/max_align.c
1

Does this need an explicit -triple=??? ?

test/Headers/max_align.c
1

I don't think this test is target-specific, I read the specifications (as opposed to the implementations) of max_align_t and __BIGGEST_ALIGNMENT__ as saying that _Alignof(max_align_t) should be the same as __BIGGEST_ALIGNMENT__.

Granted, it seems max_align_t on Windows does not match __BIGGEST_ALIGNMENT__. So, a custom malloc implementation querying on one might not align sufficiently based on the other.