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 

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);
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. 

  rG LLVM Github Monorepo



cfe-commits mailing list

Reply via email to