This is an archive of the discontinued LLVM Phabricator instance.

[clang][aarch64] Precondition isHomogeneousAggregate on isCXX14Aggregate
ClosedPublic

Authored by DavidTruby on Dec 7 2020, 3:17 AM.

Details

Summary

MSVC on WoA64 includes isCXX14Aggregate in its definition. This is de-facto
specification on that platform, so match msvc's behaviour.

Fixes: https://bugs.llvm.org/show_bug.cgi?id=47611

Co-authored-by: Peter Waller <peter.waller@arm.com>

Diff Detail

Event Timeline

DavidTruby created this revision.Dec 7 2020, 3:17 AM
DavidTruby requested review of this revision.Dec 7 2020, 3:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2020, 3:17 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Added extra tests for additional conditions and IR -> assembly tests.

Herald added a project: Restricted Project. · View Herald TranscriptDec 15 2020, 7:45 AM
DavidTruby retitled this revision from Precondition isHomogeneousAggregate on isCXX14Aggregate to [clang][aarch64] Precondition isHomogeneousAggregate on isCXX14Aggregate.Dec 15 2020, 7:49 AM

Add additional callee-side tests

rnk added a comment.Dec 15 2020, 11:44 AM

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.

clang/lib/CodeGen/CGCXXABI.cpp
320–321 ↗(On Diff #311938)

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.

clang/lib/CodeGen/TargetInfo.cpp
5086–5091

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.

5099–5101

This check seems suspect. I'd suggest constructing a test case involving zero width bitfields to see if we are compatible with MSVC.

clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp
472 ↗(On Diff #311938)

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.

493 ↗(On Diff #311938)

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.

502 ↗(On Diff #311938)

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

509–510 ↗(On Diff #311938)

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.

llvm/test/CodeGen/AArch64/arm64-windows-calls.ll
117

Please use CHECK-LABEL directives to ensure that these assembly checks are from the function you intend to match.

DavidTruby updated this revision to Diff 314356.Jan 4 2021, 4:13 AM

Refactor based on review comments

DavidTruby marked an inline comment as done.Jan 4 2021, 4:15 AM
DavidTruby added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
5099–5101

I can add tests for this but I think it should go into a separate patch as it's not behaviour changed by this patch

DavidTruby added inline comments.Jan 4 2021, 4:16 AM
clang/test/CodeGenCXX/microsoft-abi-sret-and-byval.cpp
493 ↗(On Diff #311938)

Yes, you're right. I wrote completely the opposite of what I meant here. I have corrected it now!

DavidTruby updated this revision to Diff 314357.Jan 4 2021, 4:23 AM

Switch from CHECK to CHECK-LABEL in tests.

DavidTruby marked an inline comment as done.Jan 4 2021, 4:23 AM
Harbormaster completed remote builds in B83887: Diff 314357.
rnk added a comment.Jan 4 2021, 12:12 PM

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

clang/lib/CodeGen/CGCXXABI.cpp
320–321 ↗(On Diff #311938)

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.

clang/lib/CodeGen/TargetInfo.cpp
5068–5069

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."

5086

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.

DavidTruby updated this revision to Diff 315042.Jan 6 2021, 8:51 PM

Refactor based on review

DavidTruby marked 2 inline comments as done.Jan 6 2021, 8:53 PM
DavidTruby added inline comments.
clang/lib/CodeGen/TargetInfo.cpp
5086

I agree this is much cleaner and have made the change based on your suggestion. Thanks for the review!

Suggesting a few small tweaks. Otherwise LGTM when @rnk is happy.

clang/test/CodeGenCXX/homogeneous-aggregates.cpp
130
148

Same comment (from below) about -LABEL for each of these functions. This should be a 'check'-alike and each function header should have a CHECK-LABEL.

llvm/test/CodeGen/AArch64/arm64-windows-calls.ll
117

@DavidTruby I think the intent here is:

CHECK-LABEL: < something matching the first line of the function definition >
CHECK: <thing to be checked>

CHECK-LABEL: <next function>

Such that if by some fluke some other function in the interim produces <thing to be checked>, it won't be allowed to match something produced in the subsequent function. This also renders the code safe against this problem if functions are added or moved around. I suggest having a CHECK-LABEL on each function heading, and the code within those functions should be CHECK/CHECK-NEXT as appropriate.

rnk added inline comments.Jan 7 2021, 10:00 AM
clang/lib/CodeGen/CGCXXABI.h
143

nit: I would group the new virtual method declaration after these two, since it also has to do with argument classification.

clang/lib/CodeGen/MicrosoftCXXABI.cpp
4370

I think x64 vectorcall supports HVAs, so this could change behavior there, unless you check the triple for aarch64.

Following the principle of limiting the scope of the change, this seems like the right condition:

return !CGM.getTarget().getTriple().isAArch64() || isTrivialForAArch64MSVC(CXXRD);

We can follow up with more x64 vectorcall tests later.

DavidTruby marked an inline comment as done.

Fix tests to use check-label correctly and add proper precondition on aarch64

rnk accepted this revision.Jan 11 2021, 12:14 PM

lgtm, thanks

llvm/test/CodeGen/AArch64/arm64-windows-calls.ll
117

Yep. I think you could still go further by adding a trailing colon. FileCheck matches for substrings of a line, so as written, this could match "call copy_pod", which wouldn't be what you want. The risk of that in this test is low, but it's a good best practice.

This revision is now accepted and ready to land.Jan 11 2021, 12:14 PM
peterwaller-arm accepted this revision.Jan 11 2021, 12:33 PM
peterwaller-arm added inline comments.
llvm/test/CodeGen/AArch64/arm64-windows-calls.ll
117

To echo @rnk, ideally your CHECK-LABEL statements should match the function header, and typically they're placed immediately above the function header, since that's logically what they're matching. The existence of the CHECK-LABEL primitive on every function header constrains the checks to only match within their intended function between the 'bracket' of two CHECK-LABEL statements.

; CHECK-LABEL: define {{.*}}thing
void thing() {
; CHECK: some codegen in thing
// ... do codegen here.
}