dblaikie added inline comments.
================ Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2118 + if (Packed) + UnadjustedAlignment = std::max(UnadjustedAlignment, UnpackedFieldAlign); UpdateAlignment(FieldAlign, UnpackedFieldAlign, PreferredAlign); ---------------- rjmccall wrote: > 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. I think I had a go at this over here & failed, might have some relevant notes: https://reviews.llvm.org/D118511#inline-1140212 But, yeah, would love to see it simplified, if possible - just the data point that I tried and failed recently :/ (& contributed to some of the current complexity) 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
