[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:2416
 
-  // Double and long long should be naturally aligned if possible.
+  // Double, long double (only when the target supports AIX power alignment) and
+  // long long should be naturally aligned if possible.
----------------
I suggest to clarify the binding of the parenthetical and also to make the context that the required alignment is lower than the natural alignment more explicit:
```
  // Double (and, for targets supporting AIX `power` alignment, long double) and
  // long long should be naturally aligned (despite requiring less alignment) if
  // possible.
```


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1210
 
+  auto getBaseOrPreferredAlign = [&](CharUnits UnpackedAlign) {
+    return (Packed && ((Context.getLangOpts().getClangABICompat() <=
----------------
The lambda is currently being named for what is produced based on the identity of what gets passed to it and not named for what it does.

This should be named `getPackedBaseAlignFromUnpacked` or similar.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1212
+    return (Packed && ((Context.getLangOpts().getClangABICompat() <=
+                        LangOptions::ClangABI::Ver6) ||
+                       Context.getTargetInfo().getTriple().isPS4()))
----------------
I suggest not applying "ABI compat" with Clang <= 6 on AIX. There was is no Clang <= 6 with AIX support.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1218
+
   // Clang <= 6 incorrectly applied the 'packed' attribute to base classes.
   // Per GCC's documentation, it only applies to non-static data members.
----------------
This comment belongs inside the lambda.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1235
 
+  // Do not use AIX special alignment if current base is not the first member or
+  // the struct is not a union.
----------------
Suggestion:
```
  // AIX `power` alignment does not apply the preferred alignment for non-union
  // classes if the source of the alignment (the current base in this context)
  // follows introduction of the first member with allocated space.
```


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1769
+
+  bool FoundNonOverlappingEmptyField = false;
+  bool SupportsAIXPowerAlignment =
----------------
The name is wrong for the value (the value corresponding to this name is `!IsOverlappingEmptyField`). I would suggest something like `FoundNonOverlappingEmptyFieldToHandle`.


================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1772
+      Context.getTargetInfo().defaultsToAIXPowerAlignment();
+  if (SupportsAIXPowerAlignment && !HandledFirstNonOverlappingEmptyField &&
+      !IsOverlappingEmptyField)
----------------
Move the definition of the variable here and just use the `if` condition as the initializer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79719