[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/include/clang/AST/RecordLayout.h:75
+  // can be different than Alignment in cases where it is beneficial for
+  // performance.
+  CharUnits PreferredAlignment;
----------------
nit for comment: I don't think it's related to performance nowadays, and it's more for ABI-compat reason. 


================
Comment at: clang/lib/AST/ASTContext.cpp:2409
+    return std::max(
+        ABIAlign, (unsigned)toBits(getASTRecordLayout(RD).PreferredAlignment));
+  }
----------------
static_cast instead of c cast?


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:826
+static bool isAIXLayout(const ASTContext &Context) {
+  return Context.getTargetInfo().getTriple().getOS() == llvm::Triple::AIX;
+}
----------------
We added supportsAIXPowerAlignment, so let's make use of that.
We could replace every call to isAIXLayout with supportsAIXPowerAlignment. This name is more expressive as well. 


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1225
+  CharUnits PreferredAlign =
+      (Packed && ((Context.getLangOpts().getClangABICompat() <=
+                   LangOptions::ClangABI::Ver6) ||
----------------
Use a lambda for this query would be more preferable if same logic get used twice.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1881
+  if (isAIXLayout(Context) && FieldOffset == CharUnits::Zero() &&
+      (IsUnion || NonOverlappingEmptyFieldFound)) {
+    FirstNonOverlappingEmptyFieldHandled = true;
----------------
Maybe it's a naive thought, but is it possible to replace `NonOverlappingEmptyFieldFound` with `IsOverlappingEmptyField && FieldOffsets.size() == 0`?


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1943
             getDataSize() != CharUnits::Zero())
           FieldOffset = getDataSize().alignTo(FieldAlign);
         else
----------------
hmm... Are we sure we should use FieldAlign instead of PreferredAlign here?
Same for the else below.


================
Comment at: clang/lib/Basic/Targets/PPC.h:377
 
+    if (Triple.isOSAIX()) {
+      LongDoubleWidth = 64;
----------------
nit: use "else if" with the above if statement?
If it enters one of the Triples above, it's not necessary to check if it is AIX any more. 


================
Comment at: clang/lib/Basic/Targets/PPC.h:411
 
     if (Triple.getOS() == llvm::Triple::AIX)
       SuitableAlign = 64;
----------------
This could get combined with the new if for AIX below. 


================
Comment at: clang/lib/Basic/Targets/PPC.h:419
 
+    if (Triple.isOSAIX()) {
+      LongDoubleWidth = 64;
----------------
nit: use "else if" with the above if statement?


================
Comment at: clang/test/Layout/aix-no-unique-address-with-double.cpp:62
+
+  [[no_unique_address]] Empty emp;
+  A a;
----------------
Not an action item, but just an interesting note that, in some cases(like this one) power alignment rule could reverse the benefit of having [[no_unique_address]] at the first place. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719