[Distributed] retain ad-hoc witnesses so that they are not optimized away in SIL #42252

ktoso · 2022-04-07T22:48:20Z

Since some of them (e.g. the decodeNextArgument specifically) are only used directly form IRGen, optimization thinks they are not used and may remove them. Making it impossible to use final classes (or structs sometimes?) to develop decoder types for distributed actors.

ktoso · 2022-04-07T22:48:33Z

resolves rdar://88211172 ([Distributed] SILGen must emit uses of decodeNextArgument so IRGen can get it cross module on FINAL classes)

lib/Serialization/SILFormat.h

include/swift/SIL/SILFunction.h

lib/SIL/Parser/ParseSIL.cpp

include/swift/SIL/SILFunction.h

lib/SIL/IR/SILPrinter.cpp

lib/SIL/Parser/ParseSIL.cpp

...untime/distributed_actor_cross_module_final_class_adhoc_requirement_not_optimized_away.swift

lib/Serialization/SILFormat.h

ktoso · 2022-04-12T05:09:28Z

lib/Serialization/DeserializeSIL.cpp

@@ -2956,20 +2957,20 @@ bool SILDeserializer::hasSILFunction(StringRef Name,
  IdentifierID replacedFunctionID;
  GenericSignatureID genericSigID;
  unsigned rawLinkage, isTransparent, isSerialized, isThunk,
-      isWithoutactuallyEscapingThunk, isGlobal, inlineStrategy,
+      isWithoutActuallyEscapingThunk, isGlobal, inlineStrategy,


was just a typo, might as well fix

include/swift/SIL/SILFunction.h

ktoso · 2022-04-13T08:35:41Z

@swift-ci please smoke test
@swift-ci please build toolchain macOS

I actually have trouble updating the SIL tests... will try some more later tonight.
Other than that this seems to work!?

toolchain to verify in real project where this has shown up

ktoso · 2022-04-13T08:43:20Z

@swift-ci please smoke test

ktoso · 2022-04-13T08:43:25Z

@swift-ci please build toolchain macOS

aschwaighofer · 2022-04-13T14:34:15Z

lib/SIL/IR/SILFunctionBuilder.cpp

+              systemTy->getAnyNominal());
+      assert(decoderTy);
+
+      auto decodeFunc = C.getDecodeNextArgumentOnDistributedInvocationDecoder(


Nickpick. This is a lot of code inline. Does it make sense to sink the code into a method on actor (ClassDecl?)?

So that we have something like:

if (!constant.isDistributedThunk()) return; auto *actor = decl->getDeclContext()->getSelfNominalTypeDecl(); if (!actor) return; auto decodeFunc = actor->ifActorGetDecodeNextArgumentOnDistributedInvocationDecode(); if (!decodeFunc) return; auto decodeRef = SILDeclRef(decodeFunc); ...

Cool, yeah happy to do that -- I always (over)-worry about putting things on ClassDecl but that'll make it much nicer.
I'll refactor a bit, thanks for calling it out 👍

Long method name but nicer:

if (auto decodeFunc = getAssociatedDistributedInvocationDecoderDecodeNextArgumentFunction( decl)) { auto decodeRef = SILDeclRef(decodeFunc); auto *adHocWitness = getOrCreateDeclaration(decodeFunc, decodeRef); F->setReferencedAdHocRequirementWitnessFunction(adHocWitness); }

aschwaighofer

lgtm

xedin · 2022-04-13T18:14:29Z

test/Distributed/Inputs/FakeDistributedActorSystems.swift

-public class FakeInvocationDecoder : DistributedTargetInvocationDecoder {
+
+@available(SwiftStdlib 5.7, *)
+public final class FakeInvocationDecoder: DistributedTargetInvocationDecoder {


How hard would it be to add enum and struct variants for the decoder so all possibilities are tested? I think we could gyb this and some other tests too?

I'll make a radar for it but right now I think that's a bit of a pain... I have no idea about gyb tbh

As soon as we are keeping track of that I am fine, I just don’t want it to be broken without us noticing…

Right yeah 👍

rdar://91735574

xedin

LGTM! I agree with @aschwaighofer that we should extract code for decode into a request but that could be done separately. More important I think to add tests for decoder being class/final class/struct/enum.

ktoso · 2022-04-13T22:37:22Z

Cool, will add the tests. Manually is fine tbh...

xedin · 2022-04-14T17:22:21Z

lib/SIL/IR/SILFunctionBuilder.cpp

+  } else if (constant.isDistributedThunk()) {
+    if (auto decodeFuncDecl =
+            getAssociatedDistributedInvocationDecoderDecodeNextArgumentFunction(
+                decl)) {


Nit: you can do that in the follow-up - if there is no decodeNextArgument we should fail here since the record of the thunk function is going to be ill-formed without it and that would lead to a crash in IRGen.

lib/SIL/IR/SILFunctionBuilder.cpp

xedin · 2022-04-14T17:25:00Z

@swift-ci please test

xedin · 2022-04-14T17:29:35Z

test/SILGen/distributed_thunk.swift

@@ -9,7 +9,7 @@ distributed actor DA {
 }

 extension DA {
-  // CHECK-LABEL: sil hidden [thunk] [distributed] [ossa] @$s17distributed_thunk2DAC1fyyYaKFTE : $@convention(method) @async (@guaranteed DA) -> @error Error
+  // CHECK-LABEL: sil hidden [thunk] [distributed] [ref_adhoc_requirement_witness "$s11Distributed29LocalTestingInvocationDecoderC18decodeNextArgumentxyKSeRzSERzlF"] [ossa] @$s17distributed_thunk2DAC1fyyYaKFTE : $@convention(method) @async (@guaranteed DA) -> @error Error {


I think I can make a gyb to test that struct/class/final class/enum all have ref_adhoc_requirement_witness referenced after you land these changes.

Awesome, thanks -- I'll fix the SIL test now and land it then

ktoso · 2022-04-16T02:41:13Z

@swift-ci please test

ktoso · 2022-04-17T05:35:00Z

@swift-ci please test

ktoso · 2022-04-17T06:53:33Z

This only fails in one of the Linux builds now...


(existential_type
  (type_alias_type decl=FakeDistributedActorSystems.(file).FakeActorSystem.SerializationRequirement@/home/build-user/swift/test/Distributed/Runtime/../Inputs/FakeDistributedActorSystems.swift:44:20 underlying='Codable'
    (parent=struct_type decl=FakeDistributedActorSystems.(file).FakeActorSystem@/home/build-user/swift/test/Distributed/Runtime/../Inputs/FakeDistributedActorSystems.swift:40:15)))
(existential_type
  (type_alias_type decl=FakeDistributedActorSystems.(file).FakeRoundtripActorSystem.SerializationRequirement@/home/build-user/swift/test/Distributed/Runtime/../Inputs/FakeDistributedActorSystems.swift:120:20 underlying='Codable'
    (parent=class_type decl=FakeDistributedActorSystems.(file).FakeRoundtripActorSystem@/home/build-user/swift/test/Distributed/Runtime/../Inputs/FakeDistributedActorSystems.swift:116:20)))
Please submit a bug report (https://swift.org/contributing/#reporting-bugs) and include the project and the crash backtrace.
Stack dump:
0.	Program arguments: /home/build-user/build/buildbot_linux/swift-linux-x86_64/bin/swift-frontend -frontend -c -primary-file /home/build-user/swift/test/Distributed/Runtime/distributed_actor_func_calls_remoteCall_echo.swift /home/build-user/swift/test/Distributed/Runtime/../Inputs/FakeDistributedActorSystems.swift -target x86_64-unknown-linux-gnu -disable-objc-interop -I /home/build-user/build/buildbot_linux/swift-linux-x86_64/test-linux-x86_64/Distributed/Runtime/Output/distributed_actor_func_calls_remoteCall_echo.swift.tmp -module-cache-path /home/build-user/build/buildbot_linux/swift-linux-x86_64/swift-test-results/x86_64-unknown-linux-gnu/clang-module-cache -swift-version 4 -O -define-availability "SwiftStdlib 9999:macOS 9999, iOS 9999, watchOS 9999, tvOS 9999" -define-availability "SwiftStdlib 5.0:macOS 10.14.4, iOS 12.2, watchOS 5.2, tvOS 12.2" -define-availability "SwiftStdlib 5.1:macOS 10.15, iOS 13.0, watchOS 6.0, tvOS 13.0" -define-availability "SwiftStdlib 5.2:macOS 10.15.4, iOS 13.4, watchOS 6.2, tvOS 13.4" -define-availability "SwiftStdlib 5.3:macOS 11.0, iOS 14.0, watchOS 7.0, tvOS 14.0" -define-availability "SwiftStdlib 5.4:macOS 11.3, iOS 14.5, watchOS 7.4, tvOS 14.5" -define-availability "SwiftStdlib 5.5:macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0" -define-availability "SwiftStdlib 5.6:macOS 12.3, iOS 15.4, watchOS 8.5, tvOS 15.4" -define-availability "SwiftStdlib 5.7:macOS 9999, iOS 9999, watchOS 9999, tvOS 9999" -disable-availability-checking -parse-as-library -module-name main -o /tmp/distributed_actor_func_calls_remoteCall_echo-f220f5.o
1.	Swift version 5.7-dev (LLVM 5f0021881119ab6, Swift 4088261a6916006)
2.	Compiling with effective version 4.1.50
3.	While evaluating request IRGenRequest(IR Generation for file "/home/build-user/swift/test/Distributed/Runtime/distributed_actor_func_calls_remoteCall_echo.swift")
4.	While emitting IR SIL function "@$s4main7GreeterC4echo4nameS2S_tYaKFTE".
 for 'echo(name:)' (in module 'main')
 #0 0x00000000060e2d73 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/home/build-user/build/buildbot_linux/swift-linux-x86_64/bin/swift-frontend+0x60e2d73)
 #1 0x00000000060e0abe llvm::sys::RunSignalHandlers() (/home/build-user/build/buildbot_linux/swift-linux-x86_64/bin/swift-frontend+0x60e0abe)

in optimized linux builds it seems FAILED: test/CMakeFiles/check-swift-all-optimize-linux-x86_64 while the normal -- check-swift-all-linux-x86_64 finished seems to be okey...

Could I ask you to help out here @xedin ? 🙏

xedin · 2022-04-17T07:16:36Z

I should be able to take a look on Monday. Pretty sure that it’s not Linux specific, PR testing on macOS doesn’t run optimized tests if I remember correctly.

xedin · 2022-04-18T21:26:05Z

@swift-ci please test Linux platform

…minated as unused

The symbol is always retained now, so the workaround to use witness table for non-final classes is no longer necessary.

…`decodeNextArgument`

xedin · 2022-04-19T00:28:25Z

@swift-ci please test

ktoso · 2022-04-19T02:55:16Z

lib/SILOptimizer/Analysis/CallerAnalysis.cpp

-                             canBeCalledIndirectly(f->getRepresentation())),
+      mayHaveIndirectCallers(
+          f->getDynamicallyReplacedFunction() ||
+          f->getReferencedAdHocRequirementWitnessFunction() ||


Oh i see... makes sense

ktoso

Thank you so much @xedin ! 🙇

ktoso · 2022-04-19T02:55:32Z

lib/SILOptimizer/IPO/DeadFunctionElimination.cpp

@@ -101,6 +101,9 @@ class DeadFunctionAndGlobalElimination {
    if (F->isDynamicallyReplaceable())
      return true;

+    if (F->getReferencedAdHocRequirementWitnessFunction())
+      return true;


Ah okey so here too, I thought it was an either or but Makes sense!

ktoso · 2022-04-19T02:55:40Z

lib/SILOptimizer/IPO/DeadFunctionElimination.cpp

@@ -325,6 +328,9 @@ class DeadFunctionAndGlobalElimination {

    LLVM_DEBUG(llvm::dbgs() << "    scan function " << F->getName() << '\n');

+    if (auto *adHocWitness = F->getReferencedAdHocRequirementWitnessFunction())
+      ensureAlive(adHocWitness);


ktoso · 2022-04-19T02:55:50Z

lib/IRGen/GenDistributed.cpp

-      emitVirtualMethodValue(IGF, decoderTy, SILDeclRef(decodeFn), methodTy);
+  auto methodPtr = FunctionPointer::forDirect(
+    classifyFunctionPointerKind(decodeSIL), fnPtr,
+    /*secondaryValue=*/nullptr, signature);

  return {decoder, decoderTy, witnessTable, methodPtr, methodTy};
 }


ktoso commented Apr 7, 2022

View reviewed changes

lib/Serialization/SILFormat.h Outdated Show resolved Hide resolved

ktoso force-pushed the wip-retain-dist-decode branch from 5583241 to 51ccba4 Compare April 8, 2022 10:07

ktoso commented Apr 8, 2022

View reviewed changes

include/swift/SIL/SILFunction.h Outdated Show resolved Hide resolved

xedin reviewed Apr 8, 2022

View reviewed changes

include/swift/SIL/SILFunction.h Outdated Show resolved Hide resolved

xedin reviewed Apr 8, 2022

View reviewed changes

lib/SIL/Parser/ParseSIL.cpp Outdated Show resolved Hide resolved

ktoso force-pushed the wip-retain-dist-decode branch from 51ccba4 to e53355e Compare April 11, 2022 21:29

ktoso added the distributed Feature → concurrency: distributed actor label Apr 11, 2022

ktoso force-pushed the wip-retain-dist-decode branch 7 times, most recently from 0ab1ad8 to a640186 Compare April 13, 2022 08:27

ktoso commented Apr 13, 2022

View reviewed changes

ktoso requested review from xedin and aschwaighofer April 13, 2022 08:28

ktoso force-pushed the wip-retain-dist-decode branch 2 times, most recently from e54a4d2 to b626c57 Compare April 13, 2022 08:32

ktoso commented Apr 13, 2022

View reviewed changes

include/swift/SIL/SILFunction.h Show resolved Hide resolved

aschwaighofer reviewed Apr 13, 2022

View reviewed changes

aschwaighofer approved these changes Apr 13, 2022

View reviewed changes

xedin reviewed Apr 13, 2022

View reviewed changes

xedin approved these changes Apr 13, 2022

View reviewed changes

ktoso force-pushed the wip-retain-dist-decode branch from b626c57 to d3ce178 Compare April 14, 2022 14:19

xedin approved these changes Apr 14, 2022

View reviewed changes

xedin reviewed Apr 14, 2022

View reviewed changes

ktoso force-pushed the wip-retain-dist-decode branch from d3ce178 to 00f7b6e Compare April 16, 2022 02:40

ktoso force-pushed the wip-retain-dist-decode branch from 00f7b6e to b239007 Compare April 17, 2022 05:34

xedin force-pushed the wip-retain-dist-decode branch from cecbae2 to dacd40d Compare April 18, 2022 21:25

ktoso and others added 8 commits April 18, 2022 16:53

[Distributed] Retain adhoc decodeNextArgument in distributed thunk

79ad927

fix protocols

870c712

cleanups

dadf301

review feedback, cleanup getting decode func

4e2c059

[Distributed] Temporarily simplify SIL tests; avoid a few checks

560e96f

[Distributed] SILOpt: Make sure that ad-hoc function does not get eli…

41a4cea

…minated as unused

[Distributed] IRGen: Remove a decodeNextArgument access workaround

8c8360a

The symbol is always retained now, so the workaround to use witness table for non-final classes is no longer necessary.

[Distributed] NFC: Adjust accessor tests to check for direct call to …

fc662bc

…`decodeNextArgument`

xedin force-pushed the wip-retain-dist-decode branch from dacd40d to fc662bc Compare April 19, 2022 00:28

ktoso commented Apr 19, 2022

View reviewed changes

xedin merged commit 2266a57 into swiftlang:main Apr 19, 2022

This was referenced Apr 19, 2022

[5.7][Distributed] Retain ad-hoc decodeNextArgument in distributed thunk #42451

Merged

[SILOpt] Make sure that dynamically replaced function is kept alive #42453

Merged

ktoso deleted the wip-retain-dist-decode branch April 20, 2022 09:26

xedin mentioned this pull request Apr 20, 2022

[5.7-04182022][Distributed] Retain ad-hoc decodeNextArgument in distributed thunk #42495

Merged

[Distributed] retain ad-hoc witnesses so that they are not optimized away in SIL #42252

[Distributed] retain ad-hoc witnesses so that they are not optimized away in SIL #42252

Uh oh!

Conversation

ktoso commented Apr 7, 2022

Uh oh!

ktoso commented Apr 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ktoso commented Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ktoso commented Apr 13, 2022

Uh oh!

ktoso commented Apr 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aschwaighofer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xedin left a comment

Choose a reason for hiding this comment

Uh oh!

ktoso commented Apr 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xedin commented Apr 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktoso commented Apr 16, 2022

Uh oh!

ktoso commented Apr 17, 2022

Uh oh!

ktoso commented Apr 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xedin commented Apr 17, 2022

Uh oh!

xedin commented Apr 18, 2022

Uh oh!

xedin commented Apr 19, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktoso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ktoso commented Apr 13, 2022 •

edited

Loading

ktoso commented Apr 17, 2022 •

edited

Loading