[Distributed] Fix when we invoke thunk, base it on typesystem implicit effects #39542
Conversation
ktoso
force-pushed
the
remote-calls-bad
branch
from
6695616 to
4416b13
Compare
|
Solved! We now have a shouldApplyDistributedThunk that captures the logic. |
ktoso
force-pushed
the
remote-calls-bad
branch
from
4416b13 to
ebdb061
Compare
| @@ -11,8 +11,6 @@ | |||
| // rdar://77798215 | |||
| // UNSUPPORTED: OS=windows-msvc | |||
|
|
|||
| // REQUIRES: rdar78290608 | |||
There was a problem hiding this comment.
we enable all of them in #38914
|
@swift-ci please smoke test |
|
@swift-ci please build toolchain |
ktoso
force-pushed
the
remote-calls-bad
branch
2 times, most recently
from
6fa8a24 to
85ba3fc
Compare
ktoso
force-pushed
the
remote-calls-bad
branch
from
85ba3fc to
bf0681d
Compare
|
@swift-ci please build toolchain |
|
@swift-ci please smoke test |
|
Linux Toolchain (Ubuntu 16.04) Install command |
| case ActorIsolation::DistributedActorInstance: { | ||
| case ActorIsolation::DistributedActorInstance: | ||
| markNearestCallAsImplicitly(/*setAsync*/None, /*setThrows*/false, | ||
| /*setDistributedThunk*/true); |
There was a problem hiding this comment.
I don't understand this. It doesn't seem like setThrows can ever be false while setDistributedThunk is true, because you can't do a distributed call without the potential for an error to be thrown.
There was a problem hiding this comment.
Yeah I think we can collapse the two - an implicit throws (currently) always implies that it's a distributed call. I wasn't sure if we want to keep it separate, technically there could be other reasons for an implicit throw in the future I guess...?
We also need to cause the think even if the implicit throws is not in effect because the func is throwing anyway
| @@ -2277,7 +2281,10 @@ namespace { | |||
| } else if (func->isDistributed()) { | |||
| tryMarkImplicitlyAsync(memberLoc, memberRef, context, | |||
| ImplicitActorHopTarget::forInstanceSelf()); | |||
| tryMarkImplicitlyThrows(memberLoc, memberRef, context); | |||
| tryMarkImplicitlyThrows(memberLoc, memberRef, context); | |||
| markNearestCallAsImplicitly(/*setAsync*/None, /*setThrows*/false, | |||
There was a problem hiding this comment.
Oh, this is because the tryMarkImplicitThrows is separate. Huh.
| ThrowsIsSet : 1, | ||
| Throws : 1, | ||
| ImplicitlyAsync : 1, | ||
| ImplicitlyThrows : 1, | ||
| NoAsync : 1 | ||
| NoAsync : 1, | ||
| ShouldApplyDistributedThunk : 1 |
There was a problem hiding this comment.
This needs to be set to false in the ApplyExpr constructor, or you'll end up with spuriously-distributed calls due to uninitialized memory.
There was a problem hiding this comment.
Oh man, thanks I totally forgot about that 😱
|
Merging, and will do a little more cleanup as follow-up |
|
Thank you 😊 |
|
macOS Toolchain Install command |
This solves: https://github.com/apple/swift/pull/39523/files#r719927958 which actually was not only meh style but actually wrong! It caused us to never call the distributed thunk⚠️
I also unlocked the https://github.com/apple/swift/compare/main...ktoso:remote-calls-bad?expand=1#diff-72a02f8df5425bc51b955369eac5037b49dc42971a6788d8de9dbc79e296d1f7 which shows the issue... (Long story why it was disabled... there's a PR reviving all those tests)