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 
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 

Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1943
             getDataSize() != CharUnits::Zero())
           FieldOffset = getDataSize().alignTo(FieldAlign);
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.



cfe-commits mailing list

Reply via email to