[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:2409
+    const RecordDecl *RD = RT->getDecl();
+    return std::max(ABIAlign, static_cast<unsigned>(toBits(
+                                  getASTRecordLayout(RD).PreferredAlignment)));
----------------
hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > Please add a comment regarding the situations where the `ABIAlign` value is greater than the `PreferredAlignment` value. It may be appropriate to assert that, absent those cases, the `PreferredAlignment` value is at least that of `ABIAlign`.
> It does not appear that the maximum of the two values is the correct answer:
> ```
> struct C {
>   double x;
> } c;
> typedef struct C __attribute__((__aligned__(2))) CC;
> 
> CC cc;
> extern char x[__alignof__(cc)];
> extern char x[2]; // this is okay with IBM XL C/C++
> ```
> Please add a comment regarding the situations where the ABIAlign value is greater than the PreferredAlignment value.

I added a `if` condition to guard the situation where `ABIAlign` should be returned instead of adding a comment. Please let me know if that is sufficient.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1888
+            BTy->getKind() == BuiltinType::LongDouble) {
+          PreferredAlign = CharUnits::fromQuantity(8);
+        }
----------------
hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > I believe an assertion that `PreferredAlign` was 4 would be appropriate.
> It seems that overriding the value should only be done after additional checks:
> ```
> typedef double __attribute__((__aligned__(2))) Dbl;
> struct A {
>   Dbl x;
> } a;
> extern char x[__alignof__(a)];
> extern char x[2]; // this is okay with IBM XL C/C++
> ```
> 
> I am getting concerned that the logic here overlaps quite a bit with `getPreferredTypeAlign` and refactoring to make the code here more common with `getPreferredTypeAlign` is necessary.
Fixed the typedef related cases with my new changes, and the overlaps were not a lot as I expected. So I didn't do any refactoring yet. Please let me know if you still think it's necessary to refactor the code somehow.


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

https://reviews.llvm.org/D79719