jyknight added a comment.
In the commit message, you refer to the preferred alignemtn as the "true"
alignment, but that's misleading. As discussed on the mailing list thread, it's
not true alignment at all.
================
Comment at: clang/include/clang/AST/RecordLayout.h:74
+ /// The maximum allowed field alignment. This is set by #pragma pack.
+ CharUnits MaxFieldAlignment;
+
----------------
efriedma wrote:
> If we have to keep around extra data anyway, probably better to add a field
> for the preferred alignment, so we can keep the preferred alignment handling
> together.
+1 to just storing the preferred alignment on the RecordLayout -- it already
had to compute it to compute the size-adjustment for AIX, anyhow. Then you no
longer need to store MaxFieldAlignment.
================
Comment at: clang/lib/AST/ASTContext.cpp:2506
if (!Target->allowsLargerPreferedTypeAlignment())
return ABIAlign;
----------------
I think from here on down is currently X86-specific, even though it's not
phrased that way right now.
It only applies if this target hook doesn't disable it, and if alignof(X) <
sizeof(X), for X in {double, long long, unsigned long long}. It would be good
to try to determine if there's any other platforms for which those conditions
actually exist today, other than x86-32. And then determine if this code block
actually _should_ trigger there. (I suspect not.) Then, mark this stuff as
truthfully completely-target-specific, instead of pretending otherwise, as we
do now.
================
Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:1944
+
+ uint64_t AlignValue = Context.toBits(Alignment);
+ if (isAIXLayout(Context))
----------------
name it RoundSizeTo, to indicate that this is to be used _only_ for aligning
the size, and isn't actually the alignment.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79719/new/
https://reviews.llvm.org/D79719
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
