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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
