[PATCH] D87029: [AIX] Implement AIX special bitfield related alignment rules

[Sean Fertile via Phabricator via cfe-commits]

sfertile added inline comments.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1633
+
+    if (BTy) {
+      BuiltinType::Kind BTyKind = BTy->getKind();
----------------
When can BTy be null? Should this instead be an assert?

Instead can we implement this without looking at the underlying type of the bitfield?
Considering StorageUnitSize:
* for types smaller then unsigned we always increase the storage size to that of unsigned.
* for int, and long in 32-bit the storage size is already 32 and we don't adjust it.
* for long long in 32-bit we decrease the size to 32.
* For long long and long in 64-bit we leave the storage size as 64.

This could be implemented as
```
if (StorageUnitSize <  Context.getTypeSize(Context.UnsignedIntTy) || (StorageUnitSize > Context.getTypeSize(Context.UnsignedIntTy) &&  Context.getTargetInfo().getTriple().isArch32Bit()))
  StorageUnitSize =   Context.getTypeSize(Context.UnsignedIntTy);
```

Without relying on extracting the underlying type and having to worry about the case where BTy is null.
If we can also handle `FieldAlign `without knowing the underlying type then I think we should do so and avoid trying to extract the underlying type altogether.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1638
+          BTyKind != BuiltinType::LongLong) {
+        // Only set bitfields alignment to unsigned when it does
+        // not have attribute align specified or is less than
----------------
`to unsigned` --> `to alignof(unsigned)`


================
Comment at: clang/test/Layout/aix-bitfield-alignment.cpp:1
+// RUN: %clang_cc1 -triple powerpc-ibm-aix-xcoff -fdump-record-layouts \
+// RUN:     -fsyntax-only -faix-pragma-pack -x c++ %s | \
----------------
I suggest we split the test cases which are C++ only into a sperate file, and run the cases that are valid in both C and C++ as both to ensure there are no differences between C and C++ layout fir bit fields.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D87029/new/

https://reviews.llvm.org/D87029