[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/ASTContext.cpp:2424
+      (T->isSpecificBuiltinType(BuiltinType::LongDouble) &&
+       Target->supportsAIXPowerAlignment()))
     // Don't increase the alignment if an alignment attribute was specified on a
----------------
Xiangling_L wrote:
> 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)
If the "base state" is AIX `power` alignment for a platform, I suggest that the name be `defaultsToAIXPowerAlignment`.


================
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.
----------------
Xiangling_L wrote:
> 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.
I am suggesting a change to the comment and not the name here. If both the comment and the name uses the same (possibly confusing) form to express the concept, then the comment does not aid comprehension of the code.


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

https://reviews.llvm.org/D79719