[PATCH] D79719: [AIX] Implement AIX special alignment rule about double/long double

[James Y Knight via Phabricator via cfe-commits]

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