JiruiWu marked an inline comment as done. JiruiWu added inline comments.
================ Comment at: clang/lib/AST/RecordLayoutBuilder.cpp:2118 + if (Packed) + UnadjustedAlignment = std::max(UnadjustedAlignment, UnpackedFieldAlign); UpdateAlignment(FieldAlign, UnpackedFieldAlign, PreferredAlign); ---------------- dblaikie wrote: > 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) I think the logic here is correct. ================ Comment at: clang/lib/CodeGen/TargetInfo.cpp:5806 if (!IsWinVariadic && isHomogeneousAggregate(Ty, Base, Members)) { if (Kind != AArch64ABIInfo::AAPCS) return ABIArgInfo::getDirect( ---------------- tmatheson wrote: > Should this change cover AAPCS_VFP too? This patch does not cover AAPCS_VFP because AAPCS_VFP is not listed in the ABIKind of the class AArch64ABIInfo. 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
