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

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a comment.

Thank for the update, apologies for not providing these suggestions the first time.



================
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())
----------------
rnk wrote:
> 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.
We still need a better name for this. Something like, `isTrivialForAArch64MSVC`. This isn't a test for C++14 aggregate-ness, it's the test that a specific compiler uses to decide if a record may be passed in registers.

---

After thinking about this more, making this a virtual CGCXXABI method, as I suggest below, makes more sense, because then the CGCXXABI class doesn't need to expose this very MSVC-specific method, and we don't need to hoist this code out of MicrosoftCXXABI.


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:5123
 
     // If this is a C++ record, check the bases first.
     if (const CXXRecordDecl *CXXRD = dyn_cast<CXXRecordDecl>(RD)) {
----------------
This comment is stale. We're looking for something more along the lines of, "... check the C++ properties of the record, such as base classes."


================
Comment at: clang/lib/CodeGen/TargetInfo.cpp:5137
+
+      if (!isHomogeneousAggregateForABI(CXXRD))
+        return false;
----------------
Apologies for moving the goalposts, but after re-reading the code, I feel like it would make more sense to make this a CGCXXABI virtual method. This code then would read:
  if (!getCXXABI().isPermittedToBeHomogeneousAggregate(CXXRD))
    return false;

IMO it also makes sense to move this up before checking bases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92751