Add support for calling C++ constructors #30630

martinboehme · 2020-03-25T13:44:22Z

A problem that we have to deal with when calling C++ constructors is that the ABI of a C++ constructor depends not only on the signature of the constructor but on the class on which it is defined. For example, it can be necessary to pass additional “implicit” arguments to constructors of virtual base classes.

It is therefore fundamentally not possible for SignatureExpansion (which doesn’t have access to the CXXConstructorDecl) to reliably produce the correct LLVM signature from only the type of the constructor. The approach we take is to try to get most of the way in SignatureExpansion by marking the SIL function return type with the @out attribute, representing the fact that C++ constructors take a this points to the object to be initialized.

This produces an “assumed” constructor ABI in SignatureExpansion that assumes there are no implicit arguments and that the return type of the constructor ABI is void (and indeed there is no way to represent anything else in the SIL type). If this assumed ABI doesn’t match the actual ABI, we insert a thunk in IRGen.

The thunk is marked alwaysinline, so it doesn’t incur any runtime overhead, but some compile-time overhead is required for LLVM to inline the thunk. If this turns out to be excessive, we can introduce what is essentially a peephole optimization for a full apply of a function_refthat refers to a C++ constructor and directly emit the constructor call in this case (along with any required implicit arguments) instead of calling the thunk. I’ve chosen not to tackle that in this PR though as it’s already large enough.

On some ABIs (e.g. Itanium x64), we get lucky and the ABI for a complete constructor call always matches the assumed ABI.

In addition to the core C++ constructor functionality, this PR adds some a couple of small required features:

Conversion of thin to a Clang void type
Handling of formal indirect results in SignatureExpansion::expandExternalSignatureTypes()

lib/ClangImporter/ImportDecl.cpp

lib/IRGen/GenClangType.cpp

lib/ClangImporter/ImportName.cpp

test/Interop/Cxx/class/Inputs/module.modulemap

Summary: This is needed in Swift for C++ interop -- see here for the corresponding Swift change: apple/swift#30630 As part of this change, I've had to make some changes to the interface of CGCXXABI to return the additional parameters separately rather than adding them directly to a `CallArgList`. Reviewers: rjmccall Reviewed By: rjmccall Subscribers: cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D79942

martinboehme · 2020-05-20T06:05:41Z

@swift-ci Please test

martinboehme · 2020-05-20T06:05:50Z

@swift-ci Please test Windows

martinboehme · 2020-05-20T06:15:21Z

This is now ready for review again -- sorry for the delay.

See also the discussion here:

https://forums.swift.org/t/calling-c-constructors-looking-for-feedback-on-my-approach/34787/18?u=mboehme

martinboehme · 2020-05-20T06:15:32Z

@rjmccall Not sure if you want to review this also?

swift-ci · 2020-05-20T07:23:13Z

Build failed
Swift Test OS X Platform
Git Sha - c8cd2d8b6a4b0f38c10cff1d9541c7b0a49202ef

test/Interop/Cxx/class/constructors-ir.swift

gribozavr · 2020-05-20T11:52:31Z

test/Interop/Cxx/class/constructors-ir.swift

+  // Note `this` return type and implicit "most derived" argument.
+  // MICROSOFT_X64: call %struct.HasVirtualBase* @"??0HasVirtualBase@@QEAA@UArgType@@@Z"(%struct.HasVirtualBase* %{{[0-9]+}}, i32 %{{[0-9]+}}, i32 1)
+  return HasVirtualBase(ArgType())
+}


Could you add some more tests, for cases that don't involve virtual base classes?

For example, you mentioned that constructors on some Itanium ABI variants on ARM64 iOS return 'this', so they would also need a thunk.

Also would be nice to have some tests for cases that don't need thunks.

Feel free to add some of them here if you are not using standard library types -- or in a separate file if you need standard library types.

Could you add some more tests, for cases that don't involve virtual base classes?

Done.

For example, you mentioned that constructors on some Itanium ABI variants on ARM64 iOS return 'this', so they would also need a thunk.

Some of the existing test cases already demonstrate this -- see the comments on the ITANIUM_ARM and MICROSOFT_X64 tests, which point out the this return type.

Also would be nice to have some tests for cases that don't need thunks.

This was already the case for the existing ITANIUM_X64 test. The newly added ITANIUM_X64 test also does not need a thunk.

gribozavr · 2020-05-20T11:57:25Z

test/Interop/Cxx/class/constructors-ir.swift

+  //   attributes of the constructor that was generated by Clang, which doesn't
+  //   insert these attributes.
+  //
+  // - The `this` parameter should _not_ carry a `nocapture` attribute (unlike


If the type is loadable in Swift, nocapture is permitted, I think.

In this example, the function returns the result indirectly, and the result is marked nocapture. Since we construct a C++ object in that memory, the constructor must also be nocapture.

Hm... good point.

Since there is no way in general to guarantee that the C++ constructor will be nocapture, I think the conclusion here is that when we're returning C++ objects indirectly, we should always do so without nocapture?

(We could, in principle, add the nocapture back if we can prove to ourselves that the pointer can't be captured. But I think Swift currently isn't set up to do that -- I assume the place that adds the nocapture is in SignatureExpansion, and that only sees the type of the function, not its definition.)

Well, we can probably check whether the type is trivially copyable. If it's trivially copyable, it can't be captured, because Swift is absolutely not going to promise not to move it around.

I think I see what you're saying. Swift can certainly also move non-trivially-copyable C++ types around as it pleases, but then the move constructor will notice this happening and can adjust the pointer that was previously captured by the constructor. But a trivially copyable type has no way of doing this, and so we can assume that the constructor doesn't capture this. Is this what you're getting at?

FWIW, I've checked, and Clang doesn't add nocapture to the this parameter of a constructor, even if the type is trivially copyable. I'm not sure why this is, but if Clang isn't adding the nocapture, I don't think we should be adding it either.

There's a wider discussion of what we should be doing WRT to nocapture for Swift functions that return C++ types (see https://bugs.swift.org/browse/SR-12845), but that's outside the scope of this PR.

martinboehme · 2020-05-25T14:24:16Z

@swift-ci Please test

swift-ci · 2020-05-25T14:27:00Z

Build failed
Swift Test Linux Platform
Git Sha - c8cd2d8b6a4b0f38c10cff1d9541c7b0a49202ef

swift-ci · 2020-05-25T14:27:02Z

Build failed
Swift Test OS X Platform
Git Sha - c8cd2d8b6a4b0f38c10cff1d9541c7b0a49202ef

martinboehme · 2020-05-25T14:52:08Z

@swift-ci Please test

swift-ci · 2020-05-25T16:14:39Z

Build failed
Swift Test Linux Platform
Git Sha - b0b13db62f22a0b70b660b384b6148a363e1a55d

swift-ci · 2020-05-25T16:34:48Z

Build failed
Swift Test OS X Platform
Git Sha - b0b13db62f22a0b70b660b384b6148a363e1a55d

martinboehme · 2020-05-26T09:13:28Z

I've tried to reproduce the LLDB test failures locally, but they pass, even with ASAN enabled. Restarting testing in case this was just an infrastructure failure (though I'm not exactly sure how it could be).

zoecarver · 2020-10-16T16:23:17Z

It looks like (on Windows only?) it's possible for a function template to have a CXXConstructorName if it wraps a CXXConstructorDecl. I wonder if #33053 will fix this (I doubt it). But, if that doesn't work, I think a fairly simple fix would be to just get the constructor decl out of the function template if we find a function template in that branch.

zoecarver · 2020-10-16T16:23:57Z

#33053
@swift-ci please test Windows platform

zoecarver · 2020-10-16T17:05:55Z

Yeah, that didn't work. I'll try my other idea.

…orting the function name. Sometimes, on windows, we get a function template wrapping the constructor decl. In this case, look through the function template to find the constructor decl.

zoecarver · 2020-10-16T17:24:00Z

@swift-ci please test Windows platform.

zoecarver · 2020-10-16T17:24:37Z

@swift-ci please test Windows platform.

zoecarver · 2020-10-16T19:26:42Z

@rjmccall thank you again for the help reviewing. I really appreciate your insights. Any other comments?

Hoping to land this in the next few days.

zoecarver · 2020-10-19T17:12:30Z

Unless anyone has an objection, I'm going to land this patch on Wednesday.

lattner · 2020-10-19T17:29:24Z

nice!

rjmccall · 2020-10-20T04:57:09Z

It looks like (on Windows only?) it's possible for a function template to have a CXXConstructorName if it wraps a CXXConstructorDecl.

Oh, yes, that's right. I don't think that's Windows only — any constructor template should have that characteristic.

rjmccall

LGTM. Thanks for seeing this through! Very impressive work.

zoecarver · 2020-10-20T16:29:27Z

Thanks, @rjmccall. Given that you've both approved this PR, I'm going to go ahead and land it now.

Before it lands, I want to run the benchmarks, though. As much to test #32721 as anything else. Hopefully, this swift-ci incantation will work.

zoecarver · 2020-10-20T16:29:52Z

#32721
@swift-ci please benchmark.

zoecarver · 2020-10-20T16:33:00Z

Actually, thinking about it, that won't work because even if it does correctly merge the branches before benchmarking, the benchmark will be shown as "added" so, we won't be able to get any real data from it.

swift-ci · 2020-10-20T17:34:55Z

Performance: -O

Regression	OLD	NEW	DELTA	RATIO
FlattenListFlatMap	3743	5255	+40.4%	0.71x (?)

Improvement	OLD	NEW	DELTA	RATIO
StringBuilderWithLongSubstring	1830	1670	-8.7%	1.10x (?)

Code size: -O

Performance: -Osize

Improvement	OLD	NEW	DELTA	RATIO
NSError	318	229	-28.0%	1.39x (?)

Code size: -Osize

Performance: -Onone

Regression	OLD	NEW	DELTA	RATIO
String.data.Medium	190	207	+8.9%	0.92x (?)
DictionaryBridgeToObjC_BulkAccess	198	213	+7.6%	0.93x (?)

Code size: -swiftlibs

How to read the data

The tables contain differences in performance which are larger than 8% and differences in code size which are larger than 1%.

If you see any unexpected regressions, you should consider fixing the
regressions before you merge the PR.

Noise: Sometimes the performance results (not code size!) contain false
alarms. Unexpected regressions which are marked with '(?)' are probably noise.
If you see regressions which you cannot explain you can try to run the
benchmarks again. If regressions still show up, please consult with the
performance team (@eeckstein).

Hardware Overview

  Model Name: Mac Pro
  Model Identifier: MacPro6,1
  Processor Name: 12-Core Intel Xeon E5
  Processor Speed: 2.7 GHz
  Number of Processors: 1
  Total Number of Cores: 12
  L2 Cache (per Core): 256 KB
  L3 Cache: 30 MB
  Memory: 64 GB

zoecarver · 2020-10-20T17:37:20Z

@swift-ci please test and merge.

zoecarver · 2020-10-20T17:38:49Z

@swift-ci please test and merge.

hlopko · 2020-10-25T18:59:27Z

Big congrats to both Martin and Zoe, impressive work!

This test was dissabled until apple#30630 (support calling C++ constructors) landed. That patch landed a while ago, so this test should be enabled.

Summary: This is needed in Swift for C++ interop -- see here for the corresponding Swift change: apple/swift#30630 As part of this change, I've had to make some changes to the interface of CGCXXABI to return the additional parameters separately rather than adding them directly to a `CallArgList`. Reviewers: rjmccall Reviewed By: rjmccall Subscribers: cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D79942

varungandhi-apple reviewed Mar 25, 2020

View reviewed changes

lib/ClangImporter/ImportDecl.cpp Outdated Show resolved Hide resolved

varungandhi-apple reviewed Mar 27, 2020

View reviewed changes

lib/IRGen/GenClangType.cpp Outdated Show resolved Hide resolved

martinboehme marked this pull request as draft April 21, 2020 07:54

gribozavr added the c++ interop Feature: Interoperability with C++ label Apr 24, 2020

This was referenced Apr 27, 2020

[NFC] Introduce isType and getDeclType #27195

Closed

SIL: Thread type expansion context through to function convention apis #31470

Merged

MForster reviewed May 15, 2020

View reviewed changes

lib/ClangImporter/ImportName.cpp Outdated Show resolved Hide resolved

test/Interop/Cxx/class/Inputs/module.modulemap Outdated Show resolved Hide resolved

martinboehme force-pushed the cxx-constructors branch from 3b902c2 to f5903f7 Compare May 15, 2020 12:25

martinboehme mentioned this pull request May 19, 2020

Cherry-pick from upstream: [clang] Add an API to retrieve implicit constructor arguments. apple/llvm-project#1240

Merged

martinboehme changed the title ~~Add support for calling C++ constructors [DO NOT MERGE]~~ Add support for calling C++ constructors May 20, 2020

martinboehme marked this pull request as ready for review May 20, 2020 06:05

gribozavr reviewed May 20, 2020

View reviewed changes

test/Interop/Cxx/class/constructors-ir.swift Outdated Show resolved Hide resolved

gribozavr reviewed May 20, 2020

View reviewed changes

[cxx-interop] Look through template decl to find constructor when imp…

545b44e

…orting the function name. Sometimes, on windows, we get a function template wrapping the constructor decl. In this case, look through the function template to find the constructor decl.

zoecarver force-pushed the cxx-constructors branch from b0dbeff to 545b44e Compare October 16, 2020 17:23

rjmccall approved these changes Oct 20, 2020

View reviewed changes

swift-ci merged commit 7e16e3b into apple:main Oct 20, 2020

hlopko mentioned this pull request Oct 26, 2020

[cxx-interop] Temporarily disable class-template-variadic test #34440

Merged

zoecarver mentioned this pull request Jan 22, 2021

[nfc] Enable member function templates silgen test. #35547

Merged

This was referenced Mar 31, 2020

[SR-12461] [C++] Add support for calling C++ constructors #54901

Open

[SR-13791] "Unkown decl with CXXConstructorName" assert on OpenBSD #56188

Closed

Add support for calling C++ constructors #30630

Add support for calling C++ constructors #30630

Conversation

martinboehme commented Mar 25, 2020 • edited

martinboehme commented May 20, 2020

martinboehme commented May 20, 2020

martinboehme commented May 20, 2020

martinboehme commented May 20, 2020

swift-ci commented May 20, 2020

gribozavr May 20, 2020

Choose a reason for hiding this comment

martinboehme May 25, 2020

Choose a reason for hiding this comment

gribozavr May 20, 2020 • edited

Choose a reason for hiding this comment

martinboehme May 25, 2020

Choose a reason for hiding this comment

rjmccall May 25, 2020

Choose a reason for hiding this comment

martinboehme May 25, 2020

Choose a reason for hiding this comment

martinboehme May 25, 2020

Choose a reason for hiding this comment

martinboehme commented May 25, 2020

swift-ci commented May 25, 2020

swift-ci commented May 25, 2020

martinboehme commented May 25, 2020

swift-ci commented May 25, 2020

swift-ci commented May 25, 2020

martinboehme commented May 26, 2020

zoecarver commented Oct 16, 2020

zoecarver commented Oct 16, 2020

zoecarver commented Oct 16, 2020

zoecarver commented Oct 16, 2020

zoecarver commented Oct 16, 2020

zoecarver commented Oct 16, 2020

zoecarver commented Oct 19, 2020

lattner commented Oct 19, 2020

rjmccall commented Oct 20, 2020

rjmccall left a comment

Choose a reason for hiding this comment

zoecarver commented Oct 20, 2020

zoecarver commented Oct 20, 2020

zoecarver commented Oct 20, 2020

swift-ci commented Oct 20, 2020

Performance: -O

Code size: -O

Performance: -Osize

Code size: -Osize

Performance: -Onone

Code size: -swiftlibs

zoecarver commented Oct 20, 2020

zoecarver commented Oct 20, 2020

hlopko commented Oct 25, 2020

martinboehme commented Mar 25, 2020 •

edited

gribozavr May 20, 2020 •

edited