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

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L marked 9 inline comments as done.
Xiangling_L added inline comments.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1796
+  bool FoundFirstNonOverlappingEmptyFieldToHandle =
+      DefaultsToAIXPowerAlignment && FieldOffset == CharUnits::Zero() &&
+      !HandledFirstNonOverlappingEmptyField && !IsOverlappingEmptyField;
----------------
hubert.reinterpretcast wrote:
> 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`.
> Also, since we're managing HandledFirstNonOverlappingEmptyField in non-AIX cases, we should remove the DefaultsToAIXPowerAlignment condition for what is currently named FoundFirstNonOverlappingEmptyFieldToHandle 

I am not sure if we want to remove the `DefaultsToAIXPowerAlignment` condition and bother with maintaining correct status of `HandledFirstNonOverlappingEmptyField` for other targets.

We are actually claiming `HandledFirstNonOverlappingEmptyField` is an auxiliary flag used for AIX only in its definition comments.

Besides, if we do want to manage `HandledFirstNonOverlappingEmptyField` in non-AIX cases, I noticed that we have to set this flag to `true` somewhere for objective-C++ cases. 


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1834
+    TypeInfo TI = Context.getTypeInfo(D->getType());
+    FieldAlign = Context.toCharUnitsFromBits(TI.Align);
+    AlignIsRequired = TI.AlignIsRequired;
----------------
hubert.reinterpretcast wrote:
> 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?
I am sure `getTypeInfo` can recognize the element type for `IncompleteArray`. I will update the comments.


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

https://reviews.llvm.org/D79719