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

[Xiangling Liao via Phabricator via cfe-commits]

Xiangling_L added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:2424
+      (T->isSpecificBuiltinType(BuiltinType::LongDouble) &&
+       Target->supportsAIXPowerAlignment()))
     // Don't increase the alignment if an alignment attribute was specified on a
----------------
hubert.reinterpretcast wrote:
> Does `supportsAIXPowerAlignment` express the condition we want to check here? That might be true for an implementation operating with `mac68k` alignment rules.
Yeah, `supportsAIXPowerAlignment` cannot separate the preferred alignment of double, long double between `power/natural` and `mac68k` alignment rules. But I noticed that currently, AIX target on wyvern or XL don't support `mac68k` , so maybe we should leave further changes to the patch which is gonna implement `mac68k` alignment rules? The possible solution I am thinking is we can add checking if the decl has `AlignMac68kAttr` into query to separate things out.

Another thing is that once we start supporting mac68k alignment rule(if we will), should we also change the ABI align values as well? (e.g. for double, it should be 2 instead)


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:660
+  /// When there are OverlappingEmptyFields existing in the aggregate, the
+  /// flag shows if the following first non-overlappingEmptyField has been
+  /// handled, if any.
----------------
hubert.reinterpretcast wrote:
> I suggest to replace (if correct) "non-overlappingEmptyField" with "non-empty or empty-but-non-overlapping field".
> 
Thanks for your suggestion. But I kinda prefer using `NonOverlappingEmptyField`, it is more consistent with `IsOverlappingEmptyField`. And also the equivalent name to `NonOverlappingEmptyField` is `NonOverlappingAndNonEmptyField` which is tedious I think.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1881
+  if (isAIXLayout(Context) && FieldOffset == CharUnits::Zero() &&
+      (IsUnion || NonOverlappingEmptyFieldFound)) {
+    FirstNonOverlappingEmptyFieldHandled = true;
----------------
jasonliu wrote:
> Maybe it's a naive thought, but is it possible to replace `NonOverlappingEmptyFieldFound` with `IsOverlappingEmptyField && FieldOffsets.size() == 0`?
I don't think these two work the same. `NonOverlappingEmptyFieldFound` represents the 1st non-empty and non-overlapping field in the record. `IsOverlappingEmptyField && FieldOffsets.size() == 0` represents something opposite.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1943
             getDataSize() != CharUnits::Zero())
           FieldOffset = getDataSize().alignTo(FieldAlign);
         else
----------------
jasonliu wrote:
> hmm... Are we sure we should use FieldAlign instead of PreferredAlign here?
> Same for the else below.
The way I see the `PreferredAlign` is not the `real alignment` the record uses (which I believe the community don't expect us to see it that way either). All code here should be attached to `ABI align`. Specially, on AIX, `PreferredAlign` is something we use to round up the record size, but it won't affect the record layout besides of that. So in other words, we are just tracking what `PreferredAlignment` value is here.


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

https://reviews.llvm.org/D79719