[PATCH] D92751: [clang][aarch64] Precondition isHomogeneousAggregate on isCXX14Aggregate

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Thanks for the fix. At first, misunderstood, I expected that an aggregate containing non-aggregates should be returned indirectly, and that the fix would be in the C++ ABI codepath. However, I see that is not the case. An aggregate may contain non-aggregates, and MSVC will in fact return such a type directly in X0/X1.



================
Comment at: clang/lib/CodeGen/CGCXXABI.cpp:320-321
+  //   No virtual functions
+  // Additionally, we need to ensure that there is a trivial copy assignment
+  // operator, a trivial destructor and no user-provided constructors.
+  if (RD->hasProtectedFields() || RD->hasPrivateFields())
----------------
I realize now that the name I chose (`ixCXX14Aggregate`) isn't very good because this routine also checks for trivial copy assignment and trivial destruction.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:5137-5142
+      const auto TT = CGT.getTarget().getTriple();
+      // MSVC Windows on Arm64 considers a type not HFA if it is not an
+      // aggregate according to the C++14 spec. This is not consistent with the
+      // AAPCS64, but is defacto spec on that platform.
+      if (TT.isAArch64() && TT.isWindowsMSVCEnvironment() &&
+          !CGCXXABI::isCXX14Aggregate(CXXRD))
----------------
This is target-independent code used on Mac, Linux, PPC64, etc. Please find a way to abstract over the triple check so that this is easier to read for the non-MSVC maintainer. Consider using a virtual method as is done below for isHomogeneousAggregateSmallEnough and is...BaseType.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:5158-5160
       // For compatibility with GCC, ignore empty bitfields in C++ mode.
       if (getContext().getLangOpts().CPlusPlus &&
           FD->isZeroLengthBitField(getContext()))
----------------
This check seems suspect. I'd suggest constructing a test case involving zero width bitfields to see if we are compatible with MSVC.


================
Comment at: clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp:472
+
+namespace pr47611 {
+// MSVC on Arm includes "isCXX14Aggregate" as part of its definition of
----------------
Since the code change here affects the HVA classification codepath, I suggest you test this in clang/test/CodeGenCXX/homogeneous-aggregates.cpp.

They already have a number of corner case tests for this logic that we probably need to port to port anyway. Add a new RUN line, and new checks.


================
Comment at: clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp:493
+struct Empty {};
+// A class with a base is returned in standard registers by MSVC
+struct HasEmptyBase : public Empty {
----------------
This comment doesn't seem accurate to me, it's returned indirectly in memory, right? The test for it below uses sret. In other words, this class never hits the HVA codepath because the C++ ABI marks it indirect.


================
Comment at: clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp:502
+NotCXX14Aggregate copy(NotCXX14Aggregate *x) { return *x; } // MSVC: stp x8,x9,[x0], Clang: str q0,[x0]
+// WOA64-LABEL: define dso_local [2 x i64] @"?copy at pr47611@@YA?AUNotPod at 1@PEAU21@@Z"(%"struct.pr47611::NotPod"* %x)
+NotPod copy(NotPod *x) { return *x; }
----------------
This is a surprising finding: MSVC will return an aggregate which contains non-aggregates directly in registers. So in this example we get an X0/X1 return:
https://gcc.godbolt.org/z/xcjh4M


================
Comment at: clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp:509-510
+
+Pod *pod;
+void call_copy_pod() {
+  *pod = copy(pod);
----------------
nit: I think the test case is simpler if you receive the pointer as a parameter and then forward it on. Global variables end up at the top of the LLVM IR in the output, but parameters appear inline.


================
Comment at: llvm/test/CodeGen/AArch64/arm64-windows-calls.ll:117
+  ret %struct.Pod %x1
+; CHECK: ldp d0, d1, [x0]
+}
----------------
Please use CHECK-LABEL directives to ensure that these assembly checks are from the function you intend to match.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92751