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@pr47611@@YA?AUNotPod@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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
