[TypeChecker] Implement distributed computed properties #59481

xedin · 2022-06-16T02:01:32Z

Add support for parsing distributed var <name> : <type> { ... } declarations and
type-checking them by injecting distributed thunks during Sema.

Resolves rdar://91283164

lib/AST/DistributedDecl.cpp

lib/SILGen/SILGenDistributed.cpp

ktoso · 2022-06-16T09:42:58Z

Okey so I made the tests pass but... We're missing the info that "this was a distributed thunk" and I'm sorry but I forgot what we agreed upon -- are we doing the new attribute to mark those? It would honestly be the simplest solution and also useful to inspect those ASTs then, and we might need it for the avoiding double-synthesis anyway, right @xedin?

In any case this will now pass, and let's follow up fixing the FIXME's in 1178f62 by introducing an attr or what other idea you had there 👍

ktoso · 2022-06-16T09:43:06Z

@swift-ci please test

lib/Sema/CSApply.cpp

ktoso · 2022-06-16T10:53:30Z

lib/Sema/CSApply.cpp

+        if (auto *FD = dyn_cast<FuncDecl>(decl)) {
+          thunkDecl = FD->getDistributedThunk();
+        } else {
+          thunkDecl = cast<VarDecl>(decl)->getDistributedThunk();


maybe

Suggested change

thunkDecl = cast<VarDecl>(decl)->getDistributedThunk();

} else if (let var cast<VarDecl>(decl)) {

thunkDecl = var->getDistributedThunk();

} else llvm_unreachable...

? Just in case I guess...

The validity is checked by requiresDistributedThunk so I don't really see a reason to double-check here, if anything cast is going to fail.

Gotcha, thanks!

ktoso · 2022-06-16T10:57:40Z

lib/Sema/CSApply.cpp

+        declRefExpr->setFunctionRefKind(choice.getFunctionRefKind());
+        declRefExpr->setType(thunkType);
+
+        cs.cacheType(declRefExpr);


oh that's new to me, why do we do this?

Are you talking about cacheType specifically? That's used to let finishApply know what the type was for the newly created node, since all the expressions involved in solving should have their types cached.

Yeah, that -- I see, thanks :)

lib/Sema/CSApply.cpp

lib/Sema/CodeSynthesisDistributedActor.cpp

lib/Sema/TypeCheckConcurrency.cpp

ktoso

Thank you, looking awesome.

The only follow up is that I forgot what we agreed to do about the thunks (too little sleep -_-) -- I'd be ok with the attribute to detect them, but do we want to do something else?

xedin · 2022-06-16T21:42:42Z

are we doing the new attribute to mark those? It would honestly be the simplest solution and also useful to inspect those ASTs then, and we might need it for the avoiding double-synthesis anyway, right @xedin?

Yes, that's is correct.

…thesize a thunk

The underlying mechanism has changed so the flag is no longer required.

xedin · 2022-06-17T19:13:52Z

@ktoso Tests have been fixed and new attributed simplified checking some what as well, so the changes are complete now.

xedin · 2022-06-17T19:13:58Z

@swift-ci please test

xedin · 2022-06-17T19:18:26Z

@swift-ci please test

lib/Sema/CSApply.cpp

Replace distributed member references with distributed thunks when access happens outside of distributed actor context. This significantly simplifies distributed compute properties implementation.

…le we work on getting them back

…utedThunk` Since the thunk is applied to expression during solution application, the use of this flag has shifted towards stating the fact.

…eter/capture base If distributed memeber is referenced on a base that represents a result of some expression which is not a variable, parameter, or a capture it should always be dispatched via a thunk.

…y check All the heavy lifting of checking whether reference throws and requires dispatch through a thunk is done during type-checking, so all `checkDistributedAccess` needs to do is determine whether access is distributed or not.

… code

The attribute comes handy during solution application to determine whether the call is using a distributed thunk.

xedin · 2022-06-17T19:36:18Z

@swift-ci please test

ktoso · 2022-06-17T23:03:48Z

Awesomeness, thank you Pavel :-)

ktoso · 2022-06-18T06:52:19Z

lib/Sema/CSApply.cpp

+    // let's mark the call as implicitly throwing/async.
+    if (isa<SelfApplyExpr>(apply->getFn())) {
+      auto *FD = dyn_cast<FuncDecl>(callee.getDecl());
+      if (FD && FD->isDistributedThunk()) {
        apply->setImplicitlyThrows(true);


This is very nice and simple now, came togeher nicely :)

ktoso · 2022-06-18T07:25:16Z

I'll pick it over to 5.7

DougGregor · 2022-06-22T06:08:28Z

lib/AST/Decl.cpp

@@ -7781,6 +7781,10 @@ bool AbstractFunctionDecl::isSendable() const {
  return getAttrs().hasAttribute<SendableAttr>();
 }

+bool AbstractFunctionDecl::isNonisolated() const {
+  return getAttrs().hasAttribute<NonisolatedAttr>();


This isn't a semantic definition of "nonisolated" (one should check getActorIsolation instead), so I'd prefer that we not have this function. Fortunately, it's unused, so we should remove it.

Ah I see, cool will remove 👍

…puted-properties" This reverts commit 8125a85, reversing changes made to 728971c.

[Distributed] Revert "#59481 distributed-computed-properties"

🍒[5.7][Distributed] Revert "#59481 distributed-computed-properties"

xedin added the distributed Feature → concurrency: distributed actor label Jun 16, 2022

xedin requested a review from ktoso June 16, 2022 02:01

ktoso mentioned this pull request Jun 16, 2022

[WIP] distributed computed properties #42321

Closed