Generalize and cleanup code for existential specialization.

[atrick]

Generalizes the ConcreteExistentialInfo abstraction so it can be used
both by the ExistentialSpecializer and SILCombine.

Splits OpenedArchetypeInfo apart from ConcreteExistentialInfo. Adds a
convenience wrapper around them both, ConcreteOpenedArchetypeInfo, for
use wherever we were originally using ConcreteExistentialInfo. This
allows a large amount of redundant code in ExistentialSpecializer.cpp
to be deleted.

Fixes a latent bug where the ExistentialSpecializer used a SIL
argument index as a function type parameter index (this will show up
if/when we decide to enable calls with indirect results).

There is still a bug where ExistentialSpecializer bit-casts an opened
existential to a concrete type. I'll attempt to fix that bug in a
follow-up PR.

Splits getAddressOfStackInit into getStackInitInst, This is cleaner and
allows both the ExistentialSpecializer and SILCombine to handle more
interesting cases in the future, like unconditional_checked_cast.

Creates utilities, initializeSubstitutionMap, and
initializeConcreteTypeDef to simplify an generalize
ConcreteExistentialInfo.

[atrick]

@swift-ci test.

[slavapestov]

There is still a bug where ExistentialSpecializer bit-casts an opened
existential to a concrete type.

I'm assuming it knows the existential has the concrete type -- is it still not correct to do that?

[rajbarik]

There is still a bug where ExistentialSpecializer bit-casts an opened
existential to a concrete type.

I'm assuming it knows the existential has the concrete type -- is it still not correct to do that?

I would like to understand more as well @atrick.

[rajbarik]

I see this comment on your code
"// FIXME: Inserting an unchecked_addr_cast below is incorrect. It assumes
// the same physical representation for opened existentials and the concrete
// type. This doesn't account for reabstraction. We could check the
// ConcreteType to make sure reabstraction doesn't affect its
// representation, but I'm not sure how to do that."

Can you paste an example here to make it concrete?

[rajbarik]

Overall, it looks good to me.. I have put in a couple of comments. Please have a look. Thanks a lot for doing this.

[rajbarik]

Does findInitExistential handle this GlobalAddr case now? It might be the case. I need to make sure the tests I wrote for them do not fail.

[atrick]

getStackInitInst calls findInitExistentialFromGlobalAddr. I think findInitExistentialFromGlobalAddrAndApply was just a trivial. If you have any out-of-tree test cases, please try them.

[rajbarik]

Thanks a lot for doing this. This was due from my side as the next PR cleanup :) You should also look at this other PR that also tries to create a concrete existential using SoleConformingType: #20977

Ideally, we should do all these in Existential.cpp with both ExistentialSpecializer and SILCombiner as consumers. Then we would have to pass PCA and CHA to this function.

[atrick]

The PRs look mostly complementary. In this PR, the ConcreteExistentialInfo constructor always first attempts to find an init_existential first before using the provided ConreteType--it basically avoids doing an unnecessary cast. If you end up merging first, I can change this PR so that it's the caller's job to first attempt to create CEI from an init_existential, then create a different CEI using the sole conforming type.

[atrick]

I just remembered that this PR does remove some functionality from ExistentialSpecializer:

-  /// Ignore any unconditional cast instructions. Is it Safe? Do we need to
-  /// also add UnconditionalCheckedCastAddrInst? TODO.
-  if (auto *Instance = dyn_cast<UnconditionalCheckedCastInst>(Arg)) {
-    Arg = Instance->getOperand();
-  }

My original motivation for this cleanup was that I wanted to extend ConcreteExistentialInfo to handle unconditional_addr_cast. But then I fixed the cast optimizer, so that became much less important. Also, I couldn't figure out how to bypass the cast correctly in the presence of conditional conformance. We cannot the source of the cast, which might have a generic parameter: SomeType<T> off to a witness method that has a requirement where T: AnyObject.

    // TODO: Once we have a way to introduce more constrained archetypes, handle
    // any unconditional_checked_cast that wasn't already statically eliminated.

@rajbarik None of the unit tests broke by removing this, and I can't see why we would need to handle unconditional_checked_cast here if the cast optimizer is doing its job, but you may want to run it through any of your test cases.

[atrick]

There is still a bug where ExistentialSpecializer bit-casts an opened
existential to a concrete type.

I'm assuming it knows the existential has the concrete type -- is it still not correct to do that?

@slavapestov, @rajbarik The FIXME in the code explains it:

// FIXME: Inserting an unchecked_addr_cast below is incorrect. It assumes
// the same physical representation for opened existentials and the concrete
// type. This doesn't account for reabstraction. We could check the
// ConcreteType to make sure reabstraction doesn't affect its
// representation, but I'm not sure how to do that.

When PCA/soleConformingType kicks in, we do know the existential's conforming type, but we do not know the type that was actually stored in the existential. I think it will be the maximally reabstracted form, right?

I think the reason I'm unable to create a test case is that PCA doesn't handle conformance on generic types:

  // Generic declarations are ignored.
  if (SoleConformingNTD->isGenericContext()) {
    return nullptr;

Otherwise something like this would probably miscompile:

protocol HasFoo {
  func foo()
}

extension Optional : HasFoo {
  func foo() {}
}

@inline(never)
func testFoo(_ f: HasFoo) {
  f.foo()
}

public func testLib(f: Optional<()->()>) {
  testFoo(f)
}

The CastOptimizer might have the same problem when it replaces an unconditional_checked_cast_addr with an init_existential_addr without reabstracting. I don't think SIL's CastOptimizer handles everything that the runtime's _dynamicCastToExistential does.

@jckarter I'd like to call this issue out explicitly in the code, and make the existential specialization and cast optimizer safe without handling all the special cases or implicitly relying on the caller to filter them out. I think we should have API something like: hasExistentialReabstraction(CanType) and bail out in those case. Does that make sense and do you have any suggestion for how to implement it and where it should live?

[jckarter]

That's not specific to existentials, though, you need to maximally reabstract in any situation where you're going to use unconditional_checked_cast_addr. If we don't already, we should check for that in the SIL verifier. I believe we already do when constructing existentials or binding generic types.

[atrick]

@rajbarik I'm happy to merge this now, but let me know if you want to merge any of your older PRs first that are going to have conflicts.

[atrick]

I replaced the FIXME with a test for reabstraction.

[atrick]

@swift-ci test.

[atrick]

@swift-ci test source compatibility.

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - 40cee4dcab8eb23118ef00f7803632c287bf7c68

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - 40cee4dcab8eb23118ef00f7803632c287bf7c68

[atrick]

@swift-ci test.

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - c2222fa3d08fbfcd22aee4a6618300ed926bfe3d

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - c2222fa3d08fbfcd22aee4a6618300ed926bfe3d

[rajbarik]

@atrick could you enable test running on this one? #19460. Let us merge that first and then your changes and finally a cleaner interface as you suggested. Makes sense?

I am unable to initiate test runs? Is there a way for me to get this access so that I do not wait on some one?

[rajbarik]

@atrick I merged the other one. Please go ahead and merge this one. Thanks.

[rajbarik]

@rajbarik I'm happy to merge this now, but let me know if you want to merge any of your older PRs first that are going to have conflicts.

@slavapestov @atrick One thing to note is that by inserting casts (from open to concrete type), we devirtualize the witness methods, however the existential code surrounding the open existential remains (which is actually dead code). I am thinking about writing a pass that would eliminate the existential related dead codes leading up to the cast, when possible. Makes sense?

[atrick]

@rajbarik I noticed that useless existentials (they are used only to open them) aren't being removed. Doing so would simplify witness specialization in SILCombineApplyVisitors. I even added a TODO in the code about this, but haven't investigated where it should be done yet. I think it's essentially the same problem that you're describing.

Before prototyping it, I suggest bring it up on the forums to get suggestions.

[atrick]

@rajbarik please request commit access. You just need to send an email to [email protected] with 4-5 examples of PRs that you've committed.

From https://swift.org/contributing/#contributing-code
Commit Access
Commit access is granted to contributors with a track record of submitting high-quality changes. If you would like commit access, please send an email to the code owners list with the GitHub user name that you want to use and a list of 5 non-trivial pull requests that were accepted without modifications.

Once you’ve been granted commit access, you will be able to commit to all of the GitHub repositories that host Swift.org projects. To verify that your commit access works, please make a test commit (for example, change a comment or add a blank line). The following policies apply to users with commit access:

You are granted commit-after-approval to all parts of Swift. To get approval, create a pull request. When the pull request is approved, you may merge it yourself.

You may commit an obvious change without first getting approval. The community expects you to use good judgment. Examples are reverting obviously broken patches, correcting code comments, and other minor changes.

You are allowed to commit changes without approval to the portions of Swift to which you have contributed or for which you have been assigned responsibility. Such commits must not break the build. This is a “trust but verify” policy, and commits of this nature are reviewed after being committed.

Multiple violations of these policies or a single egregious violation may cause commit access to be revoked. Even with commit access, your changes are still subject to code review. Of course, you are also encouraged to review other peoples’ changes.

[atrick]

@swift-ci test.

[atrick]

@swift-ci test source compatibility.

[atrick]

This required a nontrivial rebase but it's ready to merge now as soon as the branch is unlocked.

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - c745a082ffa123c0b71c6fc8ea499e0afb9878b6

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - c745a082ffa123c0b71c6fc8ea499e0afb9878b6

[atrick]

@swift-ci test.

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - e8ca62e70ccad7c159ae3ae447bc227a01106694

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - e8ca62e70ccad7c159ae3ae447bc227a01106694

[atrick]

@swift-ci test.

[atrick]

@swift-ci test source compatibility.

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - e8ca62e70ccad7c159ae3ae447bc227a01106694

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - e8ca62e70ccad7c159ae3ae447bc227a01106694

[atrick]

@swift-ci test.

[swift-ci]

Build failed
Swift Test Linux Platform
Git Sha - d2ffdc43cb8d303c791d9f9ac53a55b48676baea

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - d2ffdc43cb8d303c791d9f9ac53a55b48676baea