Dispatch initializers by their allocating entry point #19151

jckarter · 2018-09-05T22:46:35Z

Change convenience initializers not to show up in vtables (unless required or override-ing designated initializers)
Change convenience initializers only to have allocating entry points
Change all initializers to dispatch in vtables via their allocating entry points, since the initializing entry point is only needed for super.init chains, and those can always be statically resolved

rdar://problem/29634243

jckarter · 2018-09-05T22:46:47Z

@swift-ci Please test source compatibility

slavapestov · 2018-09-05T22:51:26Z

lib/SILGen/SILGenApply.cpp

-        loc, selfMeta.getValue(), SGF.SGM.getLoweredType(objcMetaType)));
+    SILValue convertedValue;
+    switch (metatypeRepPair(givenMetatypeRep, destMetatypeRep)) {
+    case metatypeRepPair(MetatypeRepresentation::Thick,


This is cute, but I don't think its more readable than just an if/else if chain :)

slavapestov · 2018-09-05T22:51:48Z

lib/SILGen/SILGenBridging.cpp

@@ -1377,6 +1377,20 @@ static SILFunctionType *emitObjCThunkArguments(SILGenFunction &SGF,
 void SILGenFunction::emitNativeToForeignThunk(SILDeclRef thunk) {
  assert(thunk.isForeign);
  SILDeclRef native = thunk.asForeign(false);
+
+  // If we're calling a native non-designated class initializer, we have to
+  // throw the `self` object we were given away to make a new one, since


To avoid confusion, maybe say 'deallocate' instead of 'throw'

jrose-apple

I think you'll still need some work for MissingMemberDecl (it will get simpler), but this is looking pretty reasonable to me. I'm not the SIL expert, though.

jrose-apple · 2018-09-05T23:14:08Z

lib/AST/Decl.cpp

@@ -5246,6 +5246,18 @@ static bool requiresNewVTableEntry(const AbstractFunctionDecl *decl) {
  // Dynamic methods are always accessed by objc_msgSend().
  if (decl->isFinal() || decl->isDynamic() || decl->hasClangNode())
    return false;
+
+  // Initializers are not normally inherited, but required initializers can


I'm confused by this "inherited". Requiring a new vtable entry also happens when the type changes.

A convenience initializer is always effectively final, though, so even if it overrides a designated init while changing type, it'll override an existing vtable entry, but never need a new one of its own.

Ah, sure, but this comment is about all initializers. I would have expected something like "Only designated initializers and required convenience initializers appear in the vtable; bail out if this is an initializer that is neither designated nor required."

jrose-apple · 2018-09-05T23:17:45Z

lib/SILGen/SILGenApply.cpp

      assert(isa<ClassDecl>(nominal)
             && "some new kind of init context we haven't implemented");
-      useAllocatingCtor = static_cast<bool>(SGF.AllocatorMetatype) &&
-                          !ctorRef->getDecl()->isObjC();
+      useAllocatingCtor = ctorRef->getDecl()->isImportAsMember()


This is a different test. What's special about isImportAsMember that IRGen needs to know about it? What's wrong with isFactoryInit()?

jrose-apple · 2018-09-05T23:18:57Z

lib/SILGen/SILGenConstructor.cpp

@@ -485,7 +481,7 @@ void SILGenFunction::emitClassConstructorAllocator(ConstructorDecl *ctor) {
  // Allocate the 'self' value.
  bool useObjCAllocation = usesObjCAllocator(selfClassDecl);

-  if (ctor->isConvenienceInit() || ctor->hasClangNode()) {
+  if (ctor->hasClangNode()) {
    // For a convenience initializer or an initializer synthesized
    // for an Objective-C class, allocate using the metatype.


Comment needs updating here too, right?

They should never be dynamically dispatched (unless `required`), so this entry should never be used. We were accidentally dynamically dispatching to them in convenience-to-convenience `self.init` delegations; fix that.

jckarter · 2018-09-10T22:51:32Z

@swift-ci Please test

jckarter · 2018-09-10T22:52:14Z

@jrose-apple @slavapestov mind taking another look over this now? It should be close to ready to commit.

swift-ci · 2018-09-10T22:53:21Z

Build failed
Swift Test OS X Platform
Git Sha - 60ea50e4914583421ce8fcc26a280aa0031753f8

swift-ci · 2018-09-10T23:07:22Z

Build failed
Swift Test Linux Platform
Git Sha - 60ea50e4914583421ce8fcc26a280aa0031753f8

jckarter · 2018-09-11T00:40:43Z

@swift-ci Please test

jrose-apple

Implementation looks good, some concerns about the test updates (of which there were a lot!). Also, maybe I scrolled too fast, but is there a test for the new @objc convenience initializer thunk, the one that does a dealloc_partial_ref?

jrose-apple · 2018-09-11T01:05:01Z

lib/Sema/TypeCheckDeclOverride.cpp

-                              : OverrideRequiresKeyword::Always;
+    return !ctor->isDesignatedInit() || ctor->isRequired()
+      ? OverrideRequiresKeyword::Never
+      : OverrideRequiresKeyword::Always;


Sorry, can you add parens around the condition here? I know ternary operator is low-precedence but I have to remind myself of that every time.

jrose-apple · 2018-09-11T01:08:13Z

test/IRGen/ivar_destroyer.sil

 sil public_external @alloc_NonTrivialDerived : $@convention(method) (@thick NonTrivialDerived.Type) -> @owned NonTrivialDerived
 sil public_external @$S14ivar_destroyer17NonTrivialDerivedCfE : $@convention(method) (@guaranteed NonTrivialDerived) -> ()

 sil_vtable NonTrivialDerived {
-  #NonTrivialDerived.init!initializer.1: @init_NonTrivialDerived [override]
-  #NonTrivialDerived.init!allocator.1: @alloc_NonTrivialDerived
+  #TrivialBase.init!allocator.1: @alloc_NonTrivialDerived [override]


This changed a lot from what's there before, but correctly, AFAICT. What were we getting wrong that's now fixed?

We didn't correctly follow override chains for initializing entry points, and by the previous rules, the allocating entry point wasn't established until the subclass made it required.

jrose-apple · 2018-09-11T01:09:49Z

test/SILGen/convenience_init_peer_delegation.swift

-// CHECK-NOT:     @$S32convenience_init_peer_delegation1XCACycfC
-// CHECK:         @$S32convenience_init_peer_delegation1XCACycfc
+// CHECK:         @$S32convenience_init_peer_delegation1XCACycfC
+// CHECK-NOT:     @$S32convenience_init_peer_delegation1XCACycfc


Can this be a two-pass FileCheck? That way we'll know it's not just an ordering problem if initializing entry points ever creep back in.

jrose-apple · 2018-09-11T01:11:56Z

test/SILGen/initializers.swift

  // CHECK:   [[RESULT:%.*]] = load [copy] [[PB_BOX]]
  // CHECK:   [[WRAPPED_RESULT:%.*]] = enum $Optional<FailableBaseClass>, #Optional.some!enumelt.1, [[RESULT]] : $FailableBaseClass
  // CHECK:   destroy_value [[MARKED_SELF_BOX]]
  // CHECK:   br [[EPILOG_BB:bb[0-9]+]]([[WRAPPED_RESULT]]
  //
  // CHECK: [[EPILOG_BB]]([[WRAPPED_RESULT:%.*]] : @owned $Optional<FailableBaseClass>):
  // CHECK:   return [[WRAPPED_RESULT]]
-  // CHECK: } // end sil function '$S21failable_initializers17FailableBaseClassC21failDuringDelegation2ACSgyt_tcfc'


Please update this instead of removing it; we've had problems with CHECK lines matching multiple successive function bodies.

jrose-apple · 2018-09-11T01:13:59Z

test/SILOptimizer/definite_init_diagnostics.swift

-    _ = ivar       // expected-error {{'self' used in property access 'ivar' before 'self.init' call}}
-    ivar = x       // expected-error {{'self' used in property access 'ivar' before 'self.init' call}}
+    _ = ivar       // expected-error {{'self' used before 'self.init' call}}
+    ivar = x       // expected-error* {{'self' used before 'self.init' call}}


This is a regression, no?

Definite initialization for convenience inits now uses the same code path as for delegating value type initializers, so some of the diagnostics specialized for partially-initialized class instances no longer fire. I can look into the QoI regressions after, but they're consistent with what you get for a value type delegating initializer.

That seems wrong to me, then. A value type initializer is allowed to initialize a few fields and then throw those values away, but a convenience initializer isn't.

Either way, a test case should never use * on an expected-error it's actually trying to test. Either the error is present or it's not.

I agree; like I said, I'd like to look into the diagnostic issues once the codegen changes are in, because they also affect value type initializers today, and this branch is difficult to keep up with other changes to master.

I'm not happy with this, because this isn't just a diagnostic issue; it's a language change. Convenience initializers inherently can't do everything value initializers can. But I see the point about not having a long-lived branch that changes a massive number of tests.

There's no language change, only the diagnostics changed.

To be specific, "initialize a few fields and then throw it away" still does not work on convenience initializers because accessing any field of a class requires indirection through the instance pointer, and that pointer now does not exist until a self.init call happens.

If error messages go away, that's a language change. In particular, a few lines down you're somehow permitted to call a method before calling self.init().

jrose-apple · 2018-09-11T01:14:45Z

test/SILOptimizer/definite_init_diagnostics.swift

@@ -1247,8 +1247,8 @@ class SuperConvenienceSub : SuperConvenienceBase {
    super.init(i)
  }
  public init(_ i1: Int, _ i2: Int, _ i3: Int) {
-    self.init(i1, i1)
-  }
+    self.init(i1, i1) // expected-error{{'self' used before 'super.init' call}}


Oops. I think this is meant to be written as a convenience init. I'm not sure why it was accepted before at all, honestly, but I wonder if that's a source compat issue now.

jrose-apple · 2018-09-11T01:15:52Z

test/SILOptimizer/definite_init_failable_initializers_diagnostics.swift

-      something(x) // expected-error {{'self' used inside 'catch' block reachable from self.init call}}
-      something(self.x) // expected-error {{'self' used inside 'catch' block reachable from self.init call}}
+      something(x) // expected-error* {{'self' used before 'self.init'}}
+      something(self.x) // expected-error* {{'self' used before 'self.init'}}


What's up with these?

There's special handling for catch blocks trying to access partially-initialized class instances, but it looks irrelevant when there either is an object or isn't. This is again because convenience initializers now get checked exactly the same as other delegating initializers.

jrose-apple · 2018-09-11T01:16:44Z

test/Serialization/Recovery/typedefs.swift

-// 28 CHECK-VTABLE-NEXT: #User.init!initializer.1:
-// 29 CHECK-VTABLE-NEXT: #User.init!allocator.1:
+// 27 CHECK-VTABLE-NEXT: #User.init!allocator.1:
+// 28 CHECK-VTABLE-NEXT: #User.init!allocator.1:
 // 30 CHECK-VTABLE-NEXT: #User.lastMethod!1:


Please renumber too. It's a pain to check that the GEP index is correct otherwise.

jrose-apple · 2018-09-11T01:19:59Z

lib/SILOptimizer/Mandatory/DIMemoryUseCollectorOwnership.h

+
+  /// This instruction is a load that's only used to answer a `type(of: self)`
+  /// question.
+  LoadForTypeOfSelf,


Is there a test case for this, or were tests previously failing and now they work again?

This is necessary for existing tests (as well as swiftpm and the compatibility suite) to keep working.

swift-ci · 2018-09-11T01:52:57Z

Build failed
Swift Test OS X Platform
Git Sha - 73eef4dfa2447422299312d5b07c5cc5e48bb1df

swift-ci · 2018-09-11T02:01:19Z

Build failed
Swift Test Linux Platform
Git Sha - 73eef4dfa2447422299312d5b07c5cc5e48bb1df

jckarter · 2018-09-11T03:14:02Z

@swift-ci Please clean test

jckarter · 2018-09-11T03:43:32Z

@swift-ci Please test source compatibility

swift-ci · 2018-09-11T04:16:51Z

Build failed
Swift Test OS X Platform
Git Sha - 73eef4dfa2447422299312d5b07c5cc5e48bb1df

swift-ci · 2018-09-11T04:27:31Z

Build failed
Swift Test Linux Platform
Git Sha - 73eef4dfa2447422299312d5b07c5cc5e48bb1df

jckarter · 2018-09-11T18:15:53Z

@swift-ci Please test

swift-ci · 2018-09-11T19:20:37Z

Build failed
Swift Test OS X Platform
Git Sha - 73eef4dfa2447422299312d5b07c5cc5e48bb1df

swift-ci · 2018-09-11T19:22:27Z

Build failed
Swift Test Linux Platform
Git Sha - 73eef4dfa2447422299312d5b07c5cc5e48bb1df

jckarter · 2018-09-12T22:06:40Z

@swift-ci Please test

swift-ci · 2018-09-12T22:08:51Z

Build failed
Swift Test OS X Platform
Git Sha - 73eef4dfa2447422299312d5b07c5cc5e48bb1df

swift-ci · 2018-09-12T22:14:09Z

Build failed
Swift Test Linux Platform
Git Sha - 73eef4dfa2447422299312d5b07c5cc5e48bb1df

jckarter · 2018-09-13T00:20:13Z

@swift-ci Please test source compatibility

And only dispatch designated inits by their allocating entry points. rdar://problem/29634243

… into the self argument. Before we changed convenience inits into allocating entry points, we allowed type(of: self) to be invoked on the uninitialized object, which was fine. This no longer Just Works when self doesn't even exist before `self.init` is called, but to maintain the old semantics and source compatibility, we can still just use the metatype value we were passed in.

jckarter · 2018-09-13T19:31:58Z

@swift-ci Please test

jckarter · 2018-09-13T19:32:10Z

@swift-ci Please test source compatibility

jckarter · 2018-09-13T19:32:15Z

@swift-ci Please test source compatibility

swift-ci · 2018-09-13T19:33:56Z

Build failed
Swift Test Linux Platform
Git Sha - 36a2e2f7890acfec35ae5b9953aa8b47974e64aa

swift-ci · 2018-09-13T19:35:29Z

Build failed
Swift Test OS X Platform
Git Sha - 36a2e2f7890acfec35ae5b9953aa8b47974e64aa

jckarter · 2018-09-13T19:41:34Z

@jrose-apple I think I got all your suggested updates to the tests. I ended up ripping out the changes to convenience overriding checking in the AST, because they had some fallout in the compatibility suite, and it didn't really end up simplifying the vtable formation rules because there were a lot of other things like PrintAsObjC, selector collision checking, and QoI for attempted overrides, that depended on the overrides being present in the AST and would've ended up just needing different special case behavior. How's this look now?

jrose-apple

Still a few comments but they're pretty small!

jrose-apple · 2018-09-13T20:33:13Z

test/SILGen/objc_factory_init.swift

-  // CHECK:   throw [[ERROR]]
-  // CHECK: } // end sil function '$SSo4HiveC17objc_factory_initE15otherFlakyQueenABSo3BeeC_tKcfC'
-  convenience init(otherFlakyQueen other: Bee) throws {
-    try self.init(flakyQueen: other)


What happened to this test?

I judged that it isn't testing anything unique at this point, since the convenience init behavior is consistent now with other tests for failable delegating init behavior, so I removed it.

jrose-apple · 2018-09-13T20:37:47Z

lib/SILOptimizer/Mandatory/DIMemoryUseCollectorOwnership.cpp

+    if (isa<LoadInst>(User)) {
+      auto UserVal = cast<SingleValueInstruction>(User);
+      if (UserVal->hasOneUse()
+          && isa<ValueMetatypeInst>(UserVal->getSingleUse()->get())) {


Hm. What happens if someone uses type(of: self) twice?

Each one gets its own load, at least with the way SILGen is currently implemented.

That seems worth having a test for, at least, in case SILGen ever figures out how peephole the load but not the ValueMetatypeInst.

jrose-apple · 2018-09-13T20:39:54Z

lib/SILOptimizer/Mandatory/DefiniteInitialization.cpp

+    }
+    assert(valueMetatype);
+    auto metatypeArgument = load->getFunction()->getEntryBlock()->getArguments()
+      .back();


Nitpick: load->getFunction()->getSelfMetadataArgument().

jrose-apple · 2018-09-13T21:00:28Z

The override-in-AST thing is interesting and kind of worrisome but I totally agree that it shouldn't block your progress now.

jckarter mentioned this pull request Sep 5, 2018

[WIP] SIL: Don't include convenience initializers in vtables. #19073

Closed

jckarter requested review from slavapestov and jrose-apple September 5, 2018 22:48

slavapestov reviewed Sep 5, 2018

View reviewed changes

jrose-apple reviewed Sep 5, 2018

View reviewed changes

SIL: Don't include convenience initializers in vtables.

3518089

They should never be dynamically dispatched (unless `required`), so this entry should never be used. We were accidentally dynamically dispatching to them in convenience-to-convenience `self.init` delegations; fix that.

jckarter force-pushed the allocating-convenience-initializers branch from 60ea50e to 73eef4d Compare September 10, 2018 22:51

jckarter changed the title ~~[WIP] Dispatch initializers by their allocating entry point~~ Dispatch initializers by their allocating entry point Sep 10, 2018

jrose-apple reviewed Sep 11, 2018

View reviewed changes

jckarter force-pushed the allocating-convenience-initializers branch from 73eef4d to be4eabe Compare September 12, 2018 21:49

jckarter added 3 commits September 13, 2018 12:31

SILGen: Emit convenience initializers as allocating entry points.

77a0923

And only dispatch designated inits by their allocating entry points. rdar://problem/29634243

Update tests based on review feedback.

91bf80f

jckarter force-pushed the allocating-convenience-initializers branch from 36a2e2f to 91bf80f Compare September 13, 2018 19:31

jrose-apple reviewed Sep 13, 2018

View reviewed changes

jckarter merged commit 8665342 into swiftlang:master Sep 13, 2018

jrose-apple mentioned this pull request Jan 12, 2019

Restore initializing entry points for @objc convenience initializers #21815

Merged

ktoso mentioned this pull request Aug 23, 2021

[Distributed] decode init for DistributedActor #38998

Merged

Dispatch initializers by their allocating entry point #19151

Dispatch initializers by their allocating entry point #19151

Uh oh!

Conversation

jckarter commented Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jckarter commented Sep 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrose-apple 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!

jckarter commented Sep 10, 2018

Uh oh!

jckarter commented Sep 10, 2018

Uh oh!

swift-ci commented Sep 10, 2018

Uh oh!

swift-ci commented Sep 10, 2018

Uh oh!

jckarter commented Sep 11, 2018

Uh oh!

jrose-apple 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!

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!

jckarter Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

swift-ci commented Sep 11, 2018

jckarter commented Sep 5, 2018 •

edited

Loading

jckarter Sep 11, 2018 •

edited

Loading

jckarter commented Sep 13, 2018 •

edited

Loading