SILCombine: Fix update of substitution map when substituting opened opaque archetypes #31349

aschwaighofer · 2020-04-27T21:56:46Z

When performing the substitution of the 'concrete' type that happens to
be an opened archetype we need to force the substitution to actually
call the conformance remapping function.

rdar://62202282
SR-12571

…o is true

…paque archetypes When performing the substitution of the 'concrete' type that happens to be an opened archetype we need to force the substitution to actually call the conformance remapping function. rdar://62202282 SR-12571

aschwaighofer · 2020-04-27T21:57:39Z

@swift-ci Please test

aschwaighofer · 2020-04-27T22:15:30Z

@swift-ci Please test source compatibility

slavapestov · 2020-04-27T22:24:01Z

lib/AST/ProtocolConformance.cpp

@@ -122,7 +122,8 @@ ProtocolConformanceRef::subst(Type origType,

  // Opened existentials trivially conform and do not need to go through
  // substitution map lookup.
-  if (substType->isOpenedExistential())
+  if (substType->isOpenedExistential() &&
+      !options.contains(SubstFlags::ForceSubstituteOpenedExistentials))


I'm wondering if we need the flag at all. What breaks if you just remove this check?

The reason I ask is that I'm worried there are other places where we missed passing the flag (and it's hard to discover this via grep).

Yeah, I wondered this too and started a run.

It failed in the eager specializer with an non-descriptive assert:

Assertion failed: (!isInvalid()), function getRequirement, file /Users/arnold/Stash4/swift/lib/AST/ProtocolConformance.cpp, line 79. Stack dump: 3. While evaluating request ExecuteSILPipelineRequest(Run pipelines { EarlyModulePasses, HighLevel+EarlyLoopOpt, MidModulePasses+StackPromote, MidLevel } on SIL for Swift.Swift) 4. While running pass #49681 SILModuleTransform "EagerSpecializer". 0 swift 0x00000001121a78d5 llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 37 1 swift 0x00000001121a6b25 llvm::sys::RunSignalHandlers() + 85 2 swift 0x00000001121a7ea6 SignalHandler(int) + 262 3 libsystem_platform.dylib 0x00007fff711805fd _sigtramp + 29 4 libsystem_platform.dylib 000000000000000000 _sigtramp + 18446603338618763808 5 libsystem_c.dylib 0x00007fff71056808 abort + 120 6 libsystem_c.dylib 0x00007fff71055ac6 err + 0 7 swift 0x00000001126aba51 swift::ProtocolConformanceRef::getRequirement() const (.cold.1) + 33 8 swift 0x000000010f086148 swift::ProtocolConformanceRef::getRequirement() const + 40 9 swift 0x000000010e9a4b9d swift::WitnessMethodInst::create(swift::SILDebugLocation, swift::CanType, swift::ProtocolConformanceRef, swift::SILDeclRef, swift::SILType, swift::SILFunction*, swift::SILOpenedArchetypesState&) + 125 10 swift 0x000000010e7a39f5 swift::SILCloner<swift::GenericCloner>::visitWitnessMethodInst(swift::WitnessMethodInst*) + 549 11 swift 0x000000010e7a04cc swift::SILCloner<swift::GenericCloner>::visitBlocksDepthFirst(swift::SILBasicBlock*) + 444 12 swift 0x000000010e79dc99 swift::SILCloner<swift::GenericCloner>::cloneFunctionBody(swift::SILFunction*, swift::SILBasicBlock*, llvm::ArrayRef<swift::SILValue>, bool) + 537 13 swift 0x000000010e79d7ca swift::GenericCloner::populateCloned() + 1162 14 swift 0x000000010e7c389e swift::GenericCloner::cloneFunction(swift::SILOptFunctionBuilder&, swift::SILFunction*, swift::ReabstractionInfo const&, swift::SubstitutionMap, llvm::StringRef, std::__1::function<void (swift::SILInstruction*, swift::SILInstruction*)>) + 190 15 swift 0x000000010e7c35dd swift::GenericFuncSpecializer::tryCreateSpecialization() + 317 16 swift 0x000000010e4bac3e (anonymous namespace)::EagerSpecializerTransform::run() + 1902 17 swift 0x000000010e5ad7ae swift::SILPassManager::runModulePass(unsigned int) + 558 18 swift 0x000000010e5ae10a swift::SILPassManager::execute() + 666 19 swift 0x000000010e5aa8e8 swift::SILPassManager::executePassPipelinePlan(swift::SILPassPipelinePlan const&) + 72 20 swift 0x000000010e5aa883 swift::ExecuteSILPipelineRequest::evaluate(swift::Evaluator&, swift::SILPipelineExecutionDescriptor) const + 51 21 swift 0x000000010e5c19bd swift::SimpleRequest<swift::ExecuteSILPipelineRequest, std::__1::tuple<> (swift::SILPipelineExecutionDescriptor), (swift::RequestFlags)1>::evaluateRequest(swift::ExecuteSILPipelineRequest const&, swift::Evaluator&) + 29 22 swift 0x000000010e5b3a0e llvm::Expected<swift::ExecuteSILPipelineRequest::OutputType> swift::Evaluator::getResultUncached<swift::ExecuteSILPipelineRequest>(swift::ExecuteSILPipelineRequest const&) + 366 23 swift 0x000000010e5aaa24 swift::executePassPipelinePlan(swift::SILModule*, swift::SILPassPipelinePlan const&, bool, swift::irgen::IRGenModule*) + 68 24 swift 0x000000010e5b6db7 swift::runSILOptimizationPasses(swift::SILModule&) + 151 25 swift 0x000000010e04844b swift::CompilerInstance::performSILProcessing(swift::SILModule*) + 619 26 swift 0x000000010df617ac performCompileStepsPostSILGen(swift::CompilerInstance&, swift::CompilerInvocation const&, std::__1::unique_ptr<swift::SILModule, std::__1::default_delete<swift::SILModule> >, llvm::PointerUnion<swift::ModuleDecl*, swift::SourceFile*>, swift::PrimarySpecificPaths const&, int&, swift::FrontendObserver*) + 892 27 swift 0x000000010df55bee performCompile(swift::CompilerInstance&, swift::CompilerInvocation const&, llvm::ArrayRef<char const*>, int&, swift::FrontendObserver*) + 7902 28 swift 0x000000010df52d53 swift::performFrontend(llvm::ArrayRef<char const*>, char const*, void*, swift::FrontendObserver*) + 3555

Don't know more yet.

aschwaighofer · 2020-04-27T23:09:32Z

Sigh.

15:54:03 In file included from /Applications/Xcode.app/Contents/Developer/Platforms/WatchOS.platform/Developer/SDKs/WatchOS6.2.sdk/usr/include/stdlib.h:61:
15:54:03 /Applications/Xcode.app/Contents/Developer/Platforms/WatchOS.platform/Developer/SDKs/WatchOS6.2.sdk/usr/include/Availability.h:257:10: fatal error: cannot open file '/Applications/Xcode.app/Contents/Developer/Platforms/WatchOS.platform/Developer/SDKs/WatchOS6.2.sdk/usr/include/AvailabilityInternal.h': Operation not permitted
15:54:03 #include <AvailabilityInternal.h>
15:54:03          ^
15:54:03 1 error generated.

aschwaighofer · 2020-04-27T23:09:47Z

@swift-ci Please test source compatibility debug

atrick

Thanks Arnold! It really terrifies me that we need to set this flag sometimes and I don't understand why.

aschwaighofer · 2020-04-28T22:49:48Z

You need it because for some reason we don't substitute conformances when the substituted type is an opened archetype because we assumed you don't need to.
Until we remove that condition: whenever you expect to substitute an opened archetype for another based on some optimization you would need that flag.

I am going to try to explain why we need it here:

The code first initializes a SubscriptionViewControllerDelegate existential variable with a ResourceKitProtocol existential.

A ResourceKitProtocol does not naturally conform to SubscriptionViewControllerDelegate via protocol inheritance but conforms by introducing an protocol conformance extension.

public protocol SubscriptionViewControllerDelegate { }
public protocol ResourceKitProtocol : MyObject { }

extension MyObject : SubscriptionViewControllerDelegate { }

This means that the init_existential_addr below will use a (normal_conformance type=MyObject protocol=SubscriptionViewControllerDelegate) and not an (abstract_conformance protocol=SubscriptionViewControllerDelegate) to initialize the witness table pointer.

  %5 = open_existential_ref %0 : $ResourceKitProtocol to $@opened("E4D92D2A-8893-11EA-9C89-ACDE48001122") ResourceKitProtocol
   %6 = alloc_stack $SubscriptionViewControllerDelegate
   %7 = init_existential_addr %6 : $*SubscriptionViewControllerDelegate, $@opened("E4D92D2A-8893-11EA-9C89-ACDE48001122") ResourceKitProtocol
   strong_retain %5 : $@opened("E4D92D2A-8893-11EA-9C89-ACDE48001122") ResourceKitProtocol
   store %5 to %7 : $*@opened("E4D92D2A-8893-11EA-9C89-ACDE48001122") ResourceKitProtocol

Now that the existential %6 is initialized we can just open it (%11) which will generate/make available a open type $@opened("FAA82796-8893-11EA-9C89-ACDE48001122") SubscriptionViewControllerDelegate which now carries an (abstract_conformance protocol=SubscriptionViewControllerDelegate).

If you look at the substitution map of the apply it will satisfy the requirement τ_0_0 where τ_0_0 : SubscriptionViewControllerDelegate with that abstract conformance (abstract_conformance protocol=SubscriptionViewControllerDelegate) from $@opened("FAA82796-8893-11EA-9C89-ACDE48001122") SubscriptionViewControllerDelegate.

   %10 = function_ref @callee2: $@convention(thin) <τ_0_0 where τ_0_0 : SubscriptionViewControllerDelegate> (@in τ_0_0, @thick SubscriptionViewControllerBuilder.Type) -> @owned SubscriptionViewControllerBuilder
   %11 = open_existential_addr mutable_access %6 : $*SubscriptionViewControllerDelegate to $*@opened("FAA82796-8893-11EA-9C89-ACDE48001122") SubscriptionViewControllerDelegate
   %12 = alloc_stack $@opened("FAA82796-8893-11EA-9C89-ACDE48001122") SubscriptionViewControllerDelegate
   copy_addr %11 to [initialization] %12 : $*@opened("FAA82796-8893-11EA-9C89-ACDE48001122") SubscriptionViewControllerDelegate
   %14 = apply %10<@opened("FAA82796-8893-11EA-9C89-ACDE48001122") SubscriptionViewControllerDelegate>(%12, %4) : $@convention(thin) <τ_0_0 where τ_0_0 : SubscriptionViewControllerDelegate> (@in τ_0_0, @thick SubscriptionViewControllerBuilder.Type) -> @owned SubscriptionViewControllerBuilder

The SILCombine then forwards the type @opened("E4D92D2A-8893-11EA-9C89-ACDE48001122") ResourceKitProtocol to the apply. This means the (abstract_conformance protocol=SubscriptionViewControllerDelegate) is no longer valid because ResourceKitProtocol only conforms via that protocol conformance extension (if you looked in a different module a ResourceKitProtocol would not carry the protocol witness table for SubscriptionViewControllerDelegate).

How anyone should have guessed that when working with that code? I don't know.

Trying to remove that special condition like Slava suggested would have made this work naturally. For some reason stuff crashes when I do that. I am gonna try to spend maybe a day going down that rabbit hole to see whether this is easily fixable.

atrick · 2020-04-29T00:28:40Z

@aschwaighofer that makes sense. It seems there should be a lower priority bug for removing that bail-out for substituting opened existentials if @slavapestov is willing to follow up eventually--no one else would be able to explain that code.

aschwaighofer added 2 commits April 27, 2020 13:30

Update code to print conformances if SILPrintGenericSpecializationInf…

c80776b

…o is true

aschwaighofer requested a review from atrick April 27, 2020 22:00

slavapestov approved these changes Apr 27, 2020

View reviewed changes

atrick reviewed Apr 28, 2020

View reviewed changes

aschwaighofer merged commit 0a38a47 into swiftlang:master Apr 28, 2020

aschwaighofer mentioned this pull request Apr 28, 2020

[5.3] SILCombine: Fix update of substitution map when substituting opened opaque archetypes #31385

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

SILCombine: Fix update of substitution map when substituting opened opaque archetypes #31349

SILCombine: Fix update of substitution map when substituting opened opaque archetypes #31349

Uh oh!

aschwaighofer commented Apr 27, 2020

Uh oh!

aschwaighofer commented Apr 27, 2020

Uh oh!

aschwaighofer commented Apr 27, 2020

Uh oh!

slavapestov Apr 27, 2020

Uh oh!

slavapestov Apr 27, 2020

Uh oh!

aschwaighofer Apr 27, 2020

Uh oh!

aschwaighofer commented Apr 27, 2020

Uh oh!

aschwaighofer commented Apr 27, 2020

Uh oh!

atrick left a comment

Uh oh!

aschwaighofer commented Apr 28, 2020

Uh oh!

atrick commented Apr 29, 2020

Uh oh!

Uh oh!

SILCombine: Fix update of substitution map when substituting opened opaque archetypes #31349

SILCombine: Fix update of substitution map when substituting opened opaque archetypes #31349

Uh oh!

Conversation

aschwaighofer commented Apr 27, 2020

Uh oh!

aschwaighofer commented Apr 27, 2020

Uh oh!

aschwaighofer commented Apr 27, 2020

Uh oh!

slavapestov Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

slavapestov Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

aschwaighofer Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

aschwaighofer commented Apr 27, 2020

Uh oh!

aschwaighofer commented Apr 27, 2020

Uh oh!

atrick left a comment

Choose a reason for hiding this comment

Uh oh!

aschwaighofer commented Apr 28, 2020

Uh oh!

atrick commented Apr 29, 2020

Uh oh!

Uh oh!