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

[Jason Liu via Phabricator via cfe-commits]

jasonliu added inline comments.


================
Comment at: clang/lib/AST/ASTContext.cpp:2418
+
   if (!Target->allowsLargerPreferedTypeAlignment())
     return ABIAlign;
----------------
Should this if statement go above the `if (const auto *RT = T->getAs<RecordType>()) ` ?
When Target does not allow larger prefered type alignment, then we should return ABIAlign immediately without going through the RecordType query?


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1234
+
+  bool DefaultsToAIXPowerAlignment =
+      Context.getTargetInfo().defaultsToAIXPowerAlignment();
----------------
Add const?


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1801
+
+  bool DefaultsToAIXPowerAlignment =
+      Context.getTargetInfo().defaultsToAIXPowerAlignment();
----------------
const ?


================
Comment at: clang/lib/Basic/Targets/PPC.h:425
+    } else if (Triple.isOSAIX()) {
+      SuitableAlign = 64;
+      LongDoubleWidth = 64;
----------------
SuitableAlign is set in line 412 as well. Please consider combining the two `if` statements if grouping things together makes code easier to parse.


================
Comment at: clang/test/Layout/aix-double-struct-member.cpp:2
+// RUN: %clang_cc1 -emit-llvm-only -triple powerpc-ibm-aix-xcoff \
+// RUN:     -fdump-record-layouts -fsyntax-only -x c++ %s | \
+// RUN:   FileCheck %s
----------------
You are not using ` < %s` here. So `-x c++` is redundant?


================
Comment at: clang/test/Layout/aix-double-struct-member.cpp:305
+struct A { double x; };
+struct B : A {} b;
+
----------------
`b` is not necessary when you take the `sizeof` of B below?


================
Comment at: clang/test/Layout/aix-no-unique-address-with-double.cpp:138
+// CHECK-NEXT:            |  nvsize=8, nvalign=4, preferrednvalign=4]
+
+int a = sizeof(Empty);
----------------
I think this case is interesting and may worth adding too:
```
struct F {
  [[no_unique_address]] Empty emp, emp2;
  double d;
};
```


================
Comment at: clang/test/Layout/aix-power-alignment-typedef.cpp:12
+  double x;
+} c;
+typedef struct C __attribute__((__aligned__(2))) CC;
----------------
remove `c` ?


================
Comment at: clang/test/Layout/aix-power-alignment-typedef.cpp:25
+  Dbl x;
+} a;
+
----------------
remove `a`?


================
Comment at: clang/test/Layout/aix-power-alignment-typedef.cpp:43
+  char x;
+} u;
+
----------------
remove `u`? 


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

https://reviews.llvm.org/D79719