[cxx-interop] Fix calling convention for rvalue reference params #79576
Conversation
Xazax-hun
requested review from
zoecarver,
hyp,
egorzhdan,
j-hui,
fahadnayyar and
jckarter
as code owners
Xazax-hun
force-pushed
the
gaborh/fix-consume-calling-conv
branch
from
33778fa to
0195d08
Compare
Xazax-hun
force-pushed
the
gaborh/fix-consume-calling-conv
branch
from
0195d08 to
3e2bf78
Compare
| func main() { | ||
| let x = MoveOnly() | ||
| // CHECK: MoveOnly 0 created | ||
| // CHECK-OPT: MoveOnly 0 created |
There was a problem hiding this comment.
Can you document why this CHECK-OPT needs to exist? Is it because of potentially optimized temporaries?
If that's the case, I'm wondering whether it might be worth also testing this with -fno-elide-constructors, to ensure those temporaries are forcibly created.
There was a problem hiding this comment.
Oh. I just looked at the // RUN directives and realized that OPT stands for "optimized" and not "optional". (And I thought that CHECK-OPT was built into LLVM-lit, rather than being user-defined.)
Disregard my question about what CHECK-OPT means, but I'm still wondering about -fno-elide-constructors, and I also wonder if there's a better check-prefix like CHECK-ONONE vs CHECK-DASHO that will make it obvious what is being tested.
There was a problem hiding this comment.
Updated the prefixes. -fno-elide-constructors is a Clang flag right? Here, the copies are created within Swift, so I would not expect it to make any difference. I think the main differences between optimized and non-optimized code is coming from the optimized version inlining the function defined in Swift so we no longer need some of the copies.
| func consume(_ x: consuming MoveOnly) {} | ||
| func consume(_ x: consuming Copyable) {} | ||
|
|
||
| func main() { |
There was a problem hiding this comment.
nit: can you break this test up into separate functions, and briefly document the expected behavior? It's a bit hard to keep track of which variables are allocated where.
There was a problem hiding this comment.
breaking it up into separate functions should also make it clearer which Copyable instance is being destroyed at the end.
There was a problem hiding this comment.
I split it up in 4.
| struct Copyable { | ||
| int id; | ||
| Copyable() : id(0) { printf("Copyable %d created\n", id); } | ||
| Copyable(const Copyable& other) : id(other.id + 1) { printf("Copyable %d copy-created\n", id); } |
There was a problem hiding this comment.
Is/can this copy constructor invoked when we do something like:
let c0 = Copyable()
let c1 = Copyable(c0) // or should it be Copyable(other: c0)?There was a problem hiding this comment.
No, we do not import the copy constructor as an initializer into Swift. Users cannot invoke them directly.
| @@ -3,6 +3,11 @@ module Reference { | |||
| requires cplusplus | |||
| } | |||
|
|
|||
| module SpecialMembers { | |||
There was a problem hiding this comment.
nit: why is this module case called SpecialMembers/print-special-members.h? The theme seems to be about logging copy- and move-constructor calls.
There was a problem hiding this comment.
In C++ we call move/copy constructors special member functions: https://en.wikipedia.org/wiki/Special_member_functions
| @@ -341,6 +341,7 @@ class AnalyzeForwardUse | |||
| case SILArgumentConvention::Indirect_In: | |||
| return true; | |||
| case SILArgumentConvention::Indirect_In_Guaranteed: | |||
| case SILArgumentConvention::Indirect_In_CXX: | |||
There was a problem hiding this comment.
Just to help me better understand this patch, because I don't see this case being defined in this patch, and the default case is llvm_unreachable.
Does this mean that the Indirect_In_Cxx case was not possible prior to this patch? (I'm guessing it's the additions to lib/SIL/IR/SILFunctionType.cpp that make this case possible?)
There was a problem hiding this comment.
I hit the unreachable assertion with the test case I added. My suspicion is that it was possible to trigger this even before my patch we just did not run into it with an assertions enabled compiler.
In C++, we always expected to invoke the dtor for moved-from objects. This is not the case for swift. Fortunately, @incxx calling convention is already expressing that the caller supposed to destroy the object. This fixes the missing dtor calls when calling C++ functions taking rvalue references. Fixes #77894. rdar://140786022
Xazax-hun
force-pushed
the
gaborh/fix-consume-calling-conv
branch
from
3e2bf78 to
00fa738
Compare
|
@swift-ci please smoke test |
|
@swift-ci please smoke test macOS |
|
@swift-ci please smoke test Linux |
In C++, we always expected to invoke the dtor for moved-from objects. This is not the case for swift. Fortunately, @incxx calling convention is already expressing that the caller supposed to destroy the object. This fixes the missing dtor calls when calling C++ functions taking rvalue references. Fixes #77894.
rdar://140786022