[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs and fields

tmatheson added a comment. Reverse-ping @JiruiWu 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:

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs and fields

rjmccall added a comment. Thank you. Per my comment here: > 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 > attr

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs and fields

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, Preferre

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs and fields

rjmccall requested changes to this revision. rjmccall added a comment. This revision now requires changes to proceed. Yeah. To be clear, though, I'm not asking for the overall data flow of the function to be fixed in this patch; I'm just pointing out problems in the new logic being added by thi

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs and fields

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:

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs and fields

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 fel

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs and fields

olista01 accepted this revision. olista01 added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146242/new/ https://reviews.llvm.org/D146242 ___

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs and fields

tmatheson added a comment. LGTM, but I'm not that familiar with the code that selects the alignment so it would be good to get a second opinion. Comment at: clang/lib/CodeGen/TargetInfo.cpp:5806 if (!IsWinVariadic && isHomogeneousAggregate(Ty, Base, Members)) { if (Kin

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs and fields

JiruiWu marked 2 inline comments as done. JiruiWu added inline comments. Comment at: clang/lib/CodeGen/TargetInfo.cpp:5811 +// For alignment adjusted HFAs, cap the argument alignment to 16, otherwise +// set it to 8 according to the AAPCS64 document. unsigned Align =

[PATCH] D146242: [ARM] Fixing ABI mismatch for packed structs and fields

tmatheson added a comment. Looks sensible but I don't fully understand the context of the change. Please could you explain more what is wrong with the current behaviour, and which parts of the AAPCS you are referring to. Comment at: clang/lib/CodeGen/TargetInfo.cpp:5811 +