rjmccall added inline comments.
================ Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2118 + if (Packed) + UnadjustedAlignment = std::max(UnadjustedAlignment, UnpackedFieldAlign); UpdateAlignment(FieldAlign, UnpackedFieldAlign, PreferredAlign); ---------------- I've always felt the data flow in this function was excessively convoluted. Let's puzzle it out to figure out what's going on. Ignoring the AIX stuff which I assume can't coincide with AArch64, we've got: ``` UnpackedFieldAlign = min(max(TyAlign, MaxAlignmentInChars), MaxFieldAlignment) PackedFieldAlign = min(max(1, MaxAlignmentInChars), MaxFieldAlignment) FieldAlign = FieldPacked ? PackedFieldAlign : UnpackedFieldAlign ``` where `MaxAlignmentInChars` is the highest value of all the alignment attributes on the field and `MaxFieldAlignment` is the value of `#pragma pack` that was active at the time of the struct definition. Note that this gives us `PackedFieldAlign <= FieldAlign <= UnpackedFieldAlign`. So: 1. I think it's wrong to be checking `Packed` instead of `FieldPacked` here. But: 2. If `FieldPacked`, then because `UnpackedFieldAlign >= FieldAlign`, the net effect of these three lines is `UnadjustedAlignment = std::max(UnadjustedAlignment, UnpackedFieldAlign)`. 3. If `!FieldPacked`, then `UnpackedFieldAlign == FieldAlign`, so the net effect of these three lines is *also* `UnadjustedAlignment = std::max(UnadjustedAlignment, UnpackedFieldAlign)`. 4. So actually you don't need to check `FieldPacked` at all; you should remove the old line and just do your new one unconditionally. Also, AAPCS64 seems to define UnadjustedAlignment as the "natural alignment", and there's a doc comment saying it's the max of the type alignments. That makes me wonder if we should really be considering either the `aligned` attribute or `#pragma pack` in this computation at all; maybe we should just be looking at the type alignment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146242/new/ https://reviews.llvm.org/D146242 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
