Skip to content

[Distributed] Generate SIL for DistributedActor.resolve #38938

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Aug 20, 2021
Merged

[Distributed] Generate SIL for DistributedActor.resolve #38938

merged 2 commits into from
Aug 20, 2021

Conversation

drexin
Copy link
Contributor

@drexin drexin commented Aug 18, 2021

rdar://78484431

@drexin
Copy link
Contributor Author

drexin commented Aug 18, 2021

@swift-ci smoke test

@drexin drexin requested review from ktoso and kavon August 18, 2021 20:57
@drexin drexin closed this Aug 18, 2021
@drexin drexin reopened this Aug 18, 2021
@drexin drexin force-pushed the wip-dist-resolve branch 2 times, most recently from e41627a to 20bd427 Compare August 18, 2021 21:06
@ktoso ktoso added the distributed Feature → concurrency: distributed actor label Aug 18, 2021
@drexin
Copy link
Contributor Author

drexin commented Aug 18, 2021

@swift-ci smoke test

ktoso
ktoso reviewed Aug 18, 2021
View reviewed changes
lib/SILGen/SILGenDistributed.cpp
auto switchBB = createBasicBlock();
auto errorBB = createBasicBlock();

SILFunctionConventions fnConv = F.getConventions(); // TODO: no idea?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SILFunctionConventions fnConv = F.getConventions(); // TODO: no idea?
SILFunctionConventions fnConv = F.getConventions();

seems to be right I guess :)

ktoso
ktoso reviewed Aug 18, 2021
View reviewed changes
lib/SILGen/SILGenDistributed.cpp
// --- get the uninitialized allocation from the runtime system.
FullExpr scope(Cleanups, CleanupLocation(fd));

auto optionalReturnTy = SILType::getOptionalType(returnTy);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice optionals made things a bit easier :)

ktoso
ktoso approved these changes Aug 18, 2021
View reviewed changes
Copy link
Contributor

@ktoso ktoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! We can remove the TODOs that this addressed I guess

lib/SILGen/SILGenDistributed.cpp
auto local = resolvedBB->createPhiArgument(returnTy, OwnershipKind::Owned);

B.createBranch(loc, returnBB, {local});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh nice, not too hard then 👍

lib/SILGen/SILGenDistributed.cpp
assert(transportVarDeclRefs.size() == 1);
auto *transportVarDeclRef = dyn_cast<VarDecl>(transportVarDeclRefs.front());
auto transportVarDeclRef = lookupActorTransportProperty(ctx, cd, selfValue);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

lib/SILGen/SILGenFunction.cpp

if (fd->isDistributedActorFactory()) {
// Synthesize the factory function body
emitDistributedActorFactory(fd);
} else {
prepareEpilog(true, fd->hasThrows(), CleanupLocation(fd));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, so we emit the cleanups ourselves ok 👍

stdlib/public/Distributed/ActorTransport.swift
@@ -48,7 +48,7 @@ public protocol ActorTransport: Sendable {
///
/// Detecting liveness of such remote actors shall be offered / by transport libraries
/// by other means, such as "watching an actor for termination" or similar.
func resolve<Act>(_ identity: AnyActorIdentity, as actorType: Act.Type) throws -> ActorResolved<Act> // TODO(distributed): make just optional
func resolve<Act>(_ identity: AnyActorIdentity, as actorType: Act.Type) throws -> Act? // TODO(distributed): make just optional
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func resolve<Act>(_ identity: AnyActorIdentity, as actorType: Act.Type) throws -> Act? // TODO(distributed): make just optional
func resolve<Act>(_ identity: AnyActorIdentity, as actorType: Act.Type) throws -> Act?

@drexin
Copy link
Contributor Author

drexin commented Aug 18, 2021

@swift-ci smoke test

@drexin
Copy link
Contributor Author

drexin commented Aug 19, 2021

@swift-ci smoke test

@drexin
Copy link
Contributor Author

drexin commented Aug 20, 2021

@swift-ci smoke test

ktoso
ktoso reviewed Aug 20, 2021
View reviewed changes
lib/IRGen/GenDistributed.cpp
@@ -41,9 +47,11 @@ llvm::Value *irgen::emitDistributedActorInitializeRemote(
call->setCallingConv(IGF.IGM.SwiftCC);
call->setDoesNotThrow();

out.add(call);
auto result = IGF.Builder.CreateBitCast(call, destType);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

huh I see, very good to know..! Thanks a lot!

@drexin drexin merged commit a4f3f2f into swiftlang:main Aug 20, 2021
@drexin drexin deleted the wip-dist-resolve branch August 20, 2021 04:46
ktoso added a commit to ktoso/swift that referenced this pull request Aug 23, 2021
@ktoso
Revert "Merge pull request swiftlang#38938 from drexin/wip-dist-resolve"
1950e22 
This reverts commit a4f3f2f, reversing
changes made to 8cf6c2e.
@ktoso ktoso mentioned this pull request Aug 23, 2021
Merged
ktoso added a commit that referenced this pull request Aug 23, 2021
@ktoso
Revert "Merge pull request #38938 from drexin/wip-dist-resolve" (#38994)
f6ae9f3 
This reverts commit a4f3f2f, reversing
changes made to 8cf6c2e.
drexin added a commit that referenced this pull request Aug 23, 2021
@drexin
Revert "Revert "Merge pull request #38938 from drexin/wip-dist-resolv…
d89d02d 
…e" (#38994)"

This reverts commit f6ae9f3.
ktoso added a commit to ktoso/swift that referenced this pull request Aug 23, 2021
@ktoso
Revert "Revert "Merge pull request swiftlang#38938 from drexin/wip-di…
e3e115f 
…st-resolve" (swiftlang#38994)"

This reverts commit f6ae9f3.
ktoso pushed a commit that referenced this pull request Aug 24, 2021
@drexin
Revert "Revert "Merge pull request #38938 from drexin/wip-dist-resolv…
d43ea45 
…e" (#38994)" (#39011)

This reverts commit f6ae9f3.
ktoso added a commit that referenced this pull request Aug 24, 2021
@ktoso
[Distributed] decode init for DistributedActor (#38998)
c916c0a 
* Revert "Revert "Merge pull request #38938 from drexin/wip-dist-resolve" (#38994)"

This reverts commit f6ae9f3.

* [Distributed] decode init for DistributedActor
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ktoso ktoso ktoso approved these changes

@kavon kavon Awaiting requested review from kavon kavon is a code owner

Assignees
No one assigned
Labels
distributed Feature → concurrency: distributed actor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@drexin @ktoso