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

[Tomas Matheson via Phabricator via cfe-commits]

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
+    // For alignment adjusted HFAs, cap the argument alignment to 16, otherwise
+    // set it to 8 according to the AAPCS64 document.
     unsigned Align =
----------------
Does the similar code added in D100853 need updated too?


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:5814
         getContext().getTypeUnadjustedAlignInChars(Ty).getQuantity();
-    unsigned BaseAlign = getContext().getTypeAlignInChars(Base).getQuantity();
-    Align = (Align > BaseAlign && Align >= 16) ? 16 : 0;
+    Align = (Align >= 16) ? 16 : 8;
     return ABIArgInfo::getDirect(
----------------
Does this code definitely only apply when the ABI is AAPCS64, or should there be a check for that somewhere here? I can't tell whether the `if` on line 5806 is sufficient.


================
Comment at: clang/test/CodeGen/aarch64-ABI-align-packed.cpp:1
+// RUN: %clang_cc1 -triple aarch64-arm-none-eabi \
+// RUN:   -target-feature +v8a \
----------------
Does this need the `arm` vendor in the triple?


================
Comment at: clang/test/CodeGen/aarch64-ABI-align-packed.cpp:1
+// RUN: %clang_cc1 -triple aarch64-arm-none-eabi \
+// RUN:   -target-feature +v8a \
----------------
tmatheson wrote:
> Does this need the `arm` vendor in the triple?
Please add a brief comment explaining what this is testing.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146242/new/

https://reviews.llvm.org/D146242