[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1064
     setSize(getSize() + PtrWidth);
     setDataSize(getSize());
   }
----------------
I would suggest setting `HandledFirstNonOverlappingEmptyField` to `true` here with an assertion that the current type is not a union.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1796
+  bool FoundFirstNonOverlappingEmptyFieldToHandle =
+      DefaultsToAIXPowerAlignment && FieldOffset == CharUnits::Zero() &&
+      !HandledFirstNonOverlappingEmptyField && !IsOverlappingEmptyField;
----------------
The condition is still more complex than I think it should be.

If we have found a "first" other-than-overlapping-empty-field, then we should set `HandledFirstNonOverlappingEmptyField` to `true` for non-union cases.

If `HandledFirstNonOverlappingEmptyField` being `false` is not enough for `FieldOffset == CharUnits::Zero()` to be true, then I think the correction would be to set `HandledFirstNonOverlappingEmptyField` in more places.

I would like to remove the check on `FieldOffset == CharUnits::Zero()` from here and instead have an assertion that `!HandledFirstNonOverlappingEmptyField` implies `FieldOffset == CharUnits::Zero()`.

Also, since we're managing `HandledFirstNonOverlappingEmptyField` in non-AIX cases, we should remove the `DefaultsToAIXPowerAlignment` condition for what is currently named `FoundFirstNonOverlappingEmptyFieldToHandle` (adjusting uses of it as necessary) and rename `FoundFirstNonOverlappingEmptyFieldToHandle` to `FoundFirstNonOverlappingEmptyField`.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1832
     EffectiveFieldSize = FieldSize = CharUnits::Zero();
     const ArrayType* ATy = Context.getAsArrayType(D->getType());
+    TypeInfo TI = Context.getTypeInfo(D->getType());
----------------
`ATy` seems to be an unused variable now.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1834
+    TypeInfo TI = Context.getTypeInfo(D->getType());
+    FieldAlign = Context.toCharUnitsFromBits(TI.Align);
+    AlignIsRequired = TI.AlignIsRequired;
----------------
I guess this works (we have a test for it), but the previous code made a point to use the element type and not the array type (and the comment above says we can't directly query `getTypeInfo` with the array type). @Xiangling_L, can you confirm if the comment is out-of-date and update it?


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1909
+  if (DefaultsToAIXPowerAlignment && !AlignIsRequired &&
+      (IsUnion || FoundFirstNonOverlappingEmptyFieldToHandle)) {
+    auto upgradeAlignment = [&](const BuiltinType *BTy) {
----------------
It should now be the case that `FoundFirstNonOverlappingEmptyFieldToHandle` is `true` for all union members that are not empty, meaning that the `IsUnion` part of the check only serves to admit attempts to handle types that are empty (and thus does not have subobjects that would induce an alignment upgrade).


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

https://reviews.llvm.org/D79719