Existential Specializer Pass #13991
Existential Specializer Pass #13991
Conversation
There was a problem hiding this comment.
I think the name is slightly confusing. Elsewhere in the compiler devirtualization refers to replacing virtual method calls with concrete method calls. What you're doing here is not devirtualization, it is specialization.
How about ExistentialSpecializer?
Also I think specializing existential parameters with their sole conforming class is too limited. You should replace them with a generic parameter instead.
| [&](SubstitutableType *type) -> Type { | ||
| if (GenericType2ConcreteTypeMap[type]) | ||
| return GenericType2ConcreteTypeMap[type]; | ||
| return type; |
There was a problem hiding this comment.
When does the map not contain the type parameter? That sounds like a bug.
| SILType NewSubstCalleeType; | ||
| auto FnSubsMap = | ||
| FnTy->getGenericSignature()->getSubstitutionMap(AI.getSubstitutions()); | ||
| auto FinalSubsMap = FnSubsMap.subst( |
There was a problem hiding this comment.
I would guess that FinalSubsMap would be identical to FnSubMap.
| return %2 : $Int | ||
| } | ||
|
|
||
| // CHECK-LABEL: sil shared [noinline] @$S25something_to_devirtualizeTf5n_n4main9SomeClassC_Tg5 : $@convention(thin) (@guaranteed SomeClass) -> Int { |
There was a problem hiding this comment.
I think instead of replacing the protocol parameter with the sole conforming class, it should remain a generic parameter. The generic specializer can specialize it to the class later.
As a bonus you should be able to get rid of the "sole conforming class" analysis. It makes the optimization only apply in very narrow cases, when it could be more general.
There was a problem hiding this comment.
related ticket, from the the ABI dashboard: https://bugs.swift.org/browse/SR-4331
There was a problem hiding this comment.
Just a heads up: the generic method and thunk creation now works for both protocol compositions and class-constrained compositions. I was able to eliminate the analysis altogether. I am working on the peephole optimizer right now. I will see if I can combine mine with the existing self peephole optimizer.
| auto ArgType = Args[i]->getType(); | ||
| /// Keep it simple for now. Do not handle Protocol constrained methods or Optionals. | ||
| auto SwiftArgType = ArgType.getSwiftRValueType(); | ||
| if (ArgType && ArgType.isAnyExistentialType()) { |
There was a problem hiding this comment.
isAnyExistentialType() is also true for existential metatypes. If you don't care about those, you can use isExistentialType() instead.
| /// Keep it simple for now. Do not handle Protocol constrained methods or Optionals. | ||
| auto SwiftArgType = ArgType.getSwiftRValueType(); | ||
| if (ArgType && ArgType.isAnyExistentialType()) { | ||
| auto SwiftProtoDecl = SwiftArgType.getAnyNominal(); |
There was a problem hiding this comment.
This will be null for protocol compositions, including class-constrained compositions. You should handle this more generally so that
func foo(p: P & Q & C)
Is specialized as
func foo<T>(p: T) where T : P, T : Q, T : C
|
Thanks a lot for the suggestions Slava. Let me fix them and resubmit the patch later today. |
|
@slavapestov @karwa, I committed a new version. Please review it. |
|
@rajbarik it looks when you rebased, you ended up re-applying a bunch of patches already in the repo. Please do this, Where |
|
Thank you! I will review this today. |
| @@ -299,6 +301,75 @@ class GenericFuncSpecializer { | |||
| } | |||
| }; | |||
|
|
|||
| /// A descriptor to carry information from ExistentialTransform analysis | |||
| /// to transformation. | |||
| struct ExistentialTransformArgumentDescriptor { | |||
There was a problem hiding this comment.
Maybe these structs belong in its own header file?
There was a problem hiding this comment.
Sure. Did you mean just the struct or the class ExistentialSpecializerTransform as well?
| @@ -231,14 +231,17 @@ void addSSAPasses(SILPassPipelinePlan &P, OptimizationLevelKind OpLevel) { | |||
| // Promote stack allocations to values. | |||
| P.addMem2Reg(); | |||
|
|
|||
| // Run the existentail pecializer Pass. | |||
| @@ -231,14 +231,17 @@ void addSSAPasses(SILPassPipelinePlan &P, OptimizationLevelKind OpLevel) { | |||
| // Promote stack allocations to values. | |||
| P.addMem2Reg(); | |||
|
|
|||
| // Run the existentail pecializer Pass. | |||
| P.addExistentialSpecializer(); | |||
There was a problem hiding this comment.
SSA passes run multiple times -- is that what you want? Perhaps this should be added at the same stage of the pipeline as the devirtualizer or function signature optimizations
There was a problem hiding this comment.
I do not want the pass to run multiple times, but I do depend on GenericSpecialization and SILCombiner (Concrete Type propagation) to run multiple times to see the benefit of my pass. I experimented with a few places. Do you have any suggestions (to run my pass once and make sure GenericSpecializer and SIlCombiner run more than once to get the desired result)?
| ArrayRef<Optional<ProtocolConformanceRef>> Conformance; | ||
| ArchetypeType *OpenedArchetype; | ||
| ApplyArgumentDescriptor() {} | ||
| ApplyArgumentDescriptor(SILValue NewArg, CanType ConcreteType, ArrayRef<Optional<ProtocolConformanceRef>> Conformance, ArchetypeType *OpenedArchetype) : NewArg(NewArg), ConcreteType(ConcreteType), Conformance(Conformance), OpenedArchetype(OpenedArchetype) {} |
There was a problem hiding this comment.
Lines should be < 80 chars
| struct ApplyArgumentDescriptor { | ||
| SILValue NewArg; | ||
| CanType ConcreteType; | ||
| ArrayRef<Optional<ProtocolConformanceRef>> Conformance; |
There was a problem hiding this comment.
I am storing the result of "SubMap-> lookupConformance(CanType type, ProtocolDecl *proto)" which returns an Optional as is. I do not understand why this is a problem.. Please explain.
There was a problem hiding this comment.
In the optimizer all conformances already exist, so you can assume the Optional is inhabited and dereference it right after the call to lookupConformance. Then you don't need to store Optional here.
lib/SILOptimizer/Utils/Generics.cpp
Outdated
|
|
||
| /// If the original function is generic, then maintain the same. | ||
| /// Does it have generic structure already ? | ||
| auto HasGenericSignature = FTy->getGenericSignature() != nullptr; |
There was a problem hiding this comment.
Can it ever not be generic? I thought that was the whole point of this pass.
lib/SILOptimizer/Utils/Generics.cpp
Outdated
| NewF->setUnqualifiedOwnership(); | ||
| } | ||
|
|
||
| // Transfer array attributes, if any. |
There was a problem hiding this comment.
Note the "UnqualifiedOwnership()": If the original function was Unqualified, NewF is made Unqualified too.
There was a problem hiding this comment.
I'm wondering about the part that transfers array attributes specifically
lib/SILOptimizer/Utils/Generics.cpp
Outdated
| mergeBasicBlockWithSuccessor(&(*(NewF->begin())), nullptr, nullptr); | ||
|
|
||
| /// Walk over destroy_addr instructions and perform fixups | ||
| for (const auto& mapPair : Arg2AllocStackMap) { |
There was a problem hiding this comment.
"const auto &mapPair" here and elsewhere
lib/SILOptimizer/Utils/Generics.cpp
Outdated
| NewIt++; | ||
| SILBasicBlock *OldEntryBB = &(*NewIt); | ||
|
|
||
| /// Create a fake branch instruction, which will be eliminated by the merge function below. |
There was a problem hiding this comment.
There might be a better way of doing this. I ran into a lot of SILCloner issues trying to append code from the old F to the NewF. At some point I wrote my own SILCloner too. Given that, the mergeBasicBlock eliminates everything, it should not matter. Should it?
There was a problem hiding this comment.
What issues did you run into?
lib/SILOptimizer/Utils/Generics.cpp
Outdated
| /// Get the substitutions. | ||
| CalleeGenericSig->getSubstitutions(SubMap, Substitutions); | ||
|
|
||
| /// Combine the two substitutions (original F's substitutions and now NewF). |
There was a problem hiding this comment.
Please find a way of doing this in terms of SubstitutionMaps. The representation and contents of a SubstitutionList are effectively opaque and your code will break if the internal representation's invariants change
There was a problem hiding this comment.
If I understand you correctly, you are pointing at line 2982 -- LookUpConformanceInSignature(*CalleeGenericSig));
if so, I do not quite sure how to write a lambda that contains more than one ProtocolConformanceRef, which is my case with protocol composition types. Are there any code snippets for me to look at here?
There was a problem hiding this comment.
What you want to do here is take the original apply substitutions and add substitutions for your opened existential parameters. Look at SubstitutionMap::combineSubstitutionMaps().
I'm not sure what you're suggesting about a lambda taking multiple conformances, that's not the issue here.
lib/SILOptimizer/Utils/Generics.cpp
Outdated
|
|
||
| int GPIdx = 0; | ||
| /// Convert the protocol arguments of F to generic ones. | ||
| for (auto const& IdxIt: ExistentialArgDescriptor) { |
lib/SILOptimizer/Utils/Generics.cpp
Outdated
| Requirements.push_back(NewRequirement); | ||
| } else { | ||
| auto CompType = PType->castTo<ProtocolCompositionType>(); | ||
| recursivelyAddCompositeTypes(CompType, NewGenericParam, Requirements); |
There was a problem hiding this comment.
This is the case you don't have to handle -- both ProtocolType and ProtocolCompositionType can be passed to the constructor for Requirement
There was a problem hiding this comment.
Question: if you did not add requirements explicitly one by one like above, how do you lookup conformance for them and build a conformance vector, like the one I did in 2812-2818? Is there an API for this already?
lib/SILOptimizer/Utils/Generics.cpp
Outdated
| SmallVector<Requirement, 4> Requirements; | ||
| if (PType->is<ProtocolType>()) { | ||
| auto DeclType = PType->castTo<ProtocolType>()->getDecl(); | ||
| Requirement NewRequirement(RequirementKind::Conformance, NewGenericParam, DeclType->getDeclaredType()); |
There was a problem hiding this comment.
DeclType->getDeclaredType() is going to equal PType
lib/SILOptimizer/Utils/Generics.cpp
Outdated
| } | ||
| auto Source = GenericSignatureBuilder::FloatingRequirementSource::forAbstract(); | ||
| for (auto &Req : Requirements) { | ||
| Builder.addRequirement(Req, Source, Mod); |
There was a problem hiding this comment.
Instead of adding requirements to a vector and then adding them to the builder, you can just add them directly to the builder
lib/SILOptimizer/Utils/Generics.cpp
Outdated
| InterfaceParams.reserve(params.size()); | ||
| for (auto ¶m : params) { | ||
| if (Arg2GenericTypeMap[Idx]) { | ||
| auto GenericParam = Arg2GenericTypeMap[Idx]; |
There was a problem hiding this comment.
You should just store CanTypes in this DenseMap then you won't need to call getCanonicalType()
There was a problem hiding this comment.
There are a number os usage scenario. For now, I am keeping it as is. Let me know if you see a major issue.
lib/SILOptimizer/Utils/Generics.cpp
Outdated
| llvm::SmallDenseMap<int, SILInstruction *> Arg2AllocStackMap; | ||
| for (auto &ArgDesc : ArgumentDescList) { | ||
| /// For all Generic Arguments. | ||
| if(Arg2GenericTypeMap[ArgDesc.Index]) { |
rajbarik
force-pushed
the
master
branch
4 times, most recently
from
12f210e to
5d9e366
Compare
|
@slavapestov Please have a look. Thanks. |
|
@rajbarik I think the next part of this PR I'd like to peel off and check in is the mangling for the existential->generic specialization. Can you split that part off into its own PR and we can review it and check it in? |
|
@rajbarik also my apologies for not looking at this until now, I was on vacation for the last week. |
|
Sure thing.
… On May 11, 2018, at 4:28 PM, Slava Pestov ***@***.***> wrote:
@rajbarik I think the next part of this PR I'd like to peel off and check in is the mangling for the existential->generic specialization. Can you split that part off into its own PR and we can review it and check it in?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@slavapestov Please have a look! |
|
@slavapestov When you get free time, please let me know the next steps for this PR. Thanks. |
lib/SILOptimizer/Utils/Local.cpp
Outdated
| /// %8 = alloc_stack $SomeP | ||
| /// copy_addr %3 to [initialization] %8 : $*SomeP | ||
| /// %10 = apply %9(%3) : $@convention(thin) (@in_guaranteed SomeP) | ||
| SILValue findInitExistentialFromGlobalAddr(GlobalAddrInst *GAI, |
There was a problem hiding this comment.
It looks like this got moved from the SILCombiner to Local.cpp. Can you split these kinds of refactorings into separate PRs, with justification?
There was a problem hiding this comment.
The reason I moved this is because Existential Specializer pass needs this apart from SILCombiner. I will create a PR for this. Should I just go ahead break this large PR into two more PRs, one with just the ExistentialTransform Pass and the other PR focusing on concrete type propagation? Ideas?
There was a problem hiding this comment.
There was a problem hiding this comment.
@slavapestov and others, I see a conflict on this PR in lib/SILOptimizer/Transforms/FunctionSignatureOpts.h because of the recent move from include to lib. Since I use some of the functionalities defined in FunctionSignatureOpts.h, is it ok to move it back to include?
|
@slavapestov things done in today's commit as per our conversation at WWDC: (1) squashed all commits. (2) rearranged code for ExistentialSpecializer so that they are in FunctionSignatureTransform directory with ExistentailSpecializer.cpp/h instead of sprinkled in GenericSpecializer.cpp and Generics.cpp (This is because I use FunctionSignatureOpts.h and ExistentialSpecializer is a kind of FunctionSignatureTransform as it rewrites the arguments). One thing I am still working on is to extend ExistentialTransform to generic functions. I will land it as a separate PR. Let me know how we go about breaking this PR into small pieces. Thanks. |
| break; | ||
| } | ||
| default: { | ||
| assert(true); |
There was a problem hiding this comment.
Use llvm_unreachable instead
| switch (ExistentialRepr) { | ||
| case ExistentialRepresentation::Opaque: { | ||
| archetypeValue = Builder.createOpenExistentialAddr( | ||
| Loc, OrigOperand, OpenedSILType, OpenedExistentialAccess::Mutable); |
There was a problem hiding this comment.
Does it have to be mutable? You're not specializing 'inout ExistentialType' to 'inout T' are you? For CoW existentials, mutable existentials incur a copy of the box so you don't want to do it unless you need to
| llvm::SmallVector<SILValue, 8> ApplyArgs; | ||
|
|
||
| /// Ideally this should be a SubstitutionMap. TODO. | ||
| llvm::DenseMap<SubstitutableType *, Type> Generic2OpenedTypeMap; |
There was a problem hiding this comment.
Are the keys here always GenericTypeParamTypes?
| break; | ||
| } | ||
| }; | ||
| auto GenericParam = Arg2GenericTypeMap[ArgDesc.Index]; |
There was a problem hiding this comment.
Use this pattern:
auto iter = map.find(key);
assert(iter != map.end());
... iter->second
| Generic2OpenedTypeMap[GenericParam] = OpenedType; | ||
| } else { | ||
| auto CallerArg = ThunkBody->getArgument(ArgDesc.Index); | ||
| auto SwiftType = CallerArg->getType().getASTType(); |
There was a problem hiding this comment.
This won't work if the caller has its own generic parameters. You need a way to track the generic parameters you added, only
| ArchetypeType *Opened; | ||
| auto OpenedType = | ||
| SwiftType->openAnyExistentialType(Opened)->getCanonicalType(); | ||
| Generic2OpenedTypeMap[GP] = OpenedType; |
There was a problem hiding this comment.
This OpenedType is never used?
| auto SubMap = | ||
| SubstitutionMap::get(CalleeGenericSig, | ||
| [&](SubstitutableType *type) -> Type { | ||
| return Generic2OpenedTypeMap[type]; |
There was a problem hiding this comment.
This is where you check: is 'type' a GenericTypeParamType that you added, in which case you look up into Generic2OpenedTypeMap, otherwise, you F->mapTypeIntoContext(type).
| [&](SubstitutableType *type) -> Type { | ||
| return Generic2OpenedTypeMap[type]; | ||
| }, | ||
| LookUpConformanceInSignature(*CalleeGenericSig)); |
There was a problem hiding this comment.
use MakeAbstractConformanceForGenericType() instead
|
|
||
| /// Perform the substitutions. | ||
| auto SubstCalleeType = GenCalleeType->substGenericArgs(M, SubMap); | ||
| SILType::getPrimitiveObjectType(SubstCalleeType); |
There was a problem hiding this comment.
The return value of this function is not used
|
Closing as discussed |
Existential Specializer Pass that specializes functions that take protocols (aka existentials) as arguments to transform them with concrete types instead of protocol types when the concrete types can be determined statically.