Skip to content

Commit 3ec1fe0

Browse files
slavapestovslavapestov
committed
SIL Combiner: Some fixes and improvements for partial_apply/apply peephole
- We were bailing out if the partial_apply's substitutions contained archetypes, but there was no inherent reason to do this. After fixing an issue with opened existential tracking, this started to work. - We were also bailing out if the callee was not a static function_ref. Again, there's no reason to do this, because we also emit partial_apply to form closures from class_method and witness_method calls. - There was a bug in the code for extending lifetimes of @in parameters. Even if a parameter was an input parameter to the method and not an alloc_stack, we have to copy it into a new alloc_stack, because there might be multiple invocations of an apply for a single partial_apply. - There was also a bug where we would proceed to apply the peephole to @unowned_inner_pointer functions, which is wrong. IRGen's lowering of partial_apply has special handling there and the resulting function type has an @owned result.
1 parent 06750f7 commit 3ec1fe0

File tree

4 files changed

+65
-40
lines changed

4 files changed

+65
-40
lines changed

‎lib/AST/ASTContext.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2551,6 +2551,9 @@ void ASTContext::dumpArchetypeContext(ArchetypeType *archetype,
25512551
void ASTContext::dumpArchetypeContext(ArchetypeType *archetype,
25522552
llvm::raw_ostream &os,
25532553
unsigned indent) const {
2554+
if (archetype->isOpenedExistential())
2555+
return;
2556+
25542557
archetype = archetype->getPrimary();
25552558
if (!archetype)
25562559
return;

‎lib/SIL/SILInstructions.cpp

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ static void buildTypeDependentOperands(
6868
for (auto archetype : OpenedArchetypes) {
6969
auto Def = OpenedArchetypesState.getOpenedArchetypeDef(
7070
F.getModule().Types.getLoweredType(archetype).getSwiftRValueType());
71+
assert(Def);
7172
assert(getOpenedArchetypeOf(Def->getType().getSwiftRValueType()) &&
7273
"Opened archetype operands should be of an opened existential type");
7374
TypeDependentOperands.push_back(Def);

‎lib/SILOptimizer/SILCombiner/SILCombinerApplyVisitors.cpp

Lines changed: 17 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -91,22 +91,6 @@ SILInstruction *SILCombiner::visitPartialApplyInst(PartialApplyInst *PAI) {
9191
return nullptr;
9292
}
9393

94-
static bool canCombinePartialApply(const PartialApplyInst *PAI) {
95-
// Only process partial apply if the callee is a known function.
96-
auto *FRI = dyn_cast<FunctionRefInst>(PAI->getCallee());
97-
if (!FRI)
98-
return false;
99-
100-
// Make sure that the substitution list of the PAI does not contain any
101-
// archetypes.
102-
ArrayRef<Substitution> Subs = PAI->getSubstitutions();
103-
for (Substitution S : Subs)
104-
if (S.getReplacement()->getCanonicalType()->hasArchetype())
105-
return false;
106-
107-
return true;
108-
}
109-
11094
// Helper class performing the apply{partial_apply(x,y)}(z) -> apply(z,x,y)
11195
// peephole.
11296
class PartialApplyCombiner {
@@ -132,9 +116,6 @@ class PartialApplyCombiner {
132116

133117
SILBuilder &Builder;
134118

135-
// Function referenced by partial_apply.
136-
FunctionRefInst *FRI;
137-
138119
SILCombiner *SilCombiner;
139120

140121
bool processSingleApply(FullApplySite AI);
@@ -146,7 +127,7 @@ class PartialApplyCombiner {
146127
PartialApplyCombiner(PartialApplyInst *PAI, SILBuilder &Builder,
147128
SILCombiner *SilCombiner)
148129
: isFirstTime(true), PAI(PAI), Builder(Builder),
149-
FRI(nullptr), SilCombiner(SilCombiner) {}
130+
SilCombiner(SilCombiner) {}
150131
SILInstruction *combine();
151132
};
152133

@@ -280,10 +261,8 @@ bool PartialApplyCombiner::processSingleApply(FullApplySite AI) {
280261
for (auto Op : PAI->getArguments()) {
281262
auto Arg = Op;
282263
// If there is new temporary for this argument, use it instead.
283-
if (isa<AllocStackInst>(Arg)) {
284-
if (ArgToTmp.count(Arg)) {
285-
Op = ArgToTmp.lookup(Arg);
286-
}
264+
if (ArgToTmp.count(Arg)) {
265+
Op = ArgToTmp.lookup(Arg);
287266
}
288267
Args.push_back(Op);
289268
}
@@ -314,24 +293,23 @@ bool PartialApplyCombiner::processSingleApply(FullApplySite AI) {
314293
}
315294
}
316295

317-
auto *F = FRI->getReferencedFunction();
318-
SILType FnType = F->getLoweredType();
319-
SILType ResultTy = F->getLoweredFunctionType()->getSILResult();
296+
auto Callee = PAI->getCallee();
297+
auto FnType = PAI->getSubstCalleeSILType();
298+
SILType ResultTy = PAI->getSubstCalleeType()->getSILResult();
320299
ArrayRef<Substitution> Subs = PAI->getSubstitutions();
321-
if (!Subs.empty()) {
322-
FnType = FnType.substGenericArgs(PAI->getModule(), Subs);
323-
ResultTy = FnType.getAs<SILFunctionType>()->getSILResult();
324-
}
300+
301+
// The partial_apply might be substituting in an open existential type.
302+
Builder.addOpenedArchetypeOperands(PAI);
325303

326304
FullApplySite NAI;
327305
if (auto *TAI = dyn_cast<TryApplyInst>(AI))
328306
NAI =
329-
Builder.createTryApply(AI.getLoc(), FRI, FnType, Subs, Args,
307+
Builder.createTryApply(AI.getLoc(), Callee, FnType, Subs, Args,
330308
TAI->getNormalBB(), TAI->getErrorBB());
331309
else
332310
NAI =
333-
Builder.createApply(AI.getLoc(), FRI, FnType, ResultTy, Subs, Args,
334-
cast<ApplyInst>(AI)->isNonThrowing());
311+
Builder.createApply(AI.getLoc(), Callee, FnType, ResultTy, Subs, Args,
312+
cast<ApplyInst>(AI)->isNonThrowing());
335313

336314
// We also need to release the partial_apply instruction itself because it
337315
// is consumed by the apply_instruction.
@@ -365,11 +343,11 @@ bool PartialApplyCombiner::processSingleApply(FullApplySite AI) {
365343
/// by iterating over all uses of the partial_apply and searching
366344
/// for the pattern to transform.
367345
SILInstruction *PartialApplyCombiner::combine() {
368-
if (!canCombinePartialApply(PAI))
369-
return nullptr;
370-
371-
// Only process partial apply if the callee is a known function.
372-
FRI = dyn_cast<FunctionRefInst>(PAI->getCallee());
346+
// We need to model @unowned_inner_pointer better before we can do the
347+
// peephole here.
348+
for (auto R : PAI->getSubstCalleeType()->getAllResults())
349+
if (R.getConvention() == ResultConvention::UnownedInnerPointer)
350+
return nullptr;
373351

374352
// Iterate over all uses of the partial_apply
375353
// and look for applies that use it as a callee.

‎test/SILOptimizer/sil_combine.sil

Lines changed: 44 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,10 @@ enum AddressOnlyEnum {
2929
case AddressOnly(Any)
3030
}
3131

32+
protocol FakeProtocol {
33+
func requirement()
34+
}
35+
3236
sil [global_init] @global_init_fun : $@convention(thin) () -> Builtin.RawPointer
3337

3438
sil @user : $@convention(thin) (@owned Builtin.NativeObject) -> ()
@@ -2745,11 +2749,50 @@ bb1:
27452749

27462750
bb2:
27472751
dealloc_stack %s1 : $*Bool
2748-
%r = tuple()
2752+
%r = tuple ()
2753+
return %r : $()
2754+
}
2755+
2756+
// CHECK-LABEL: sil @test_generic_partial_apply_apply
2757+
// CHECK: bb0([[ARG0:%.*]] : $*T, [[ARG1:%.*]] : $*T):
2758+
// CHECK-NEXT: [[TMP:%.*]] = alloc_stack $T
2759+
// CHECK: [[FN:%.*]] = function_ref @generic_callee
2760+
// CHECK-NEXT: copy_addr [[ARG1]] to [initialization] [[TMP]] : $*T
2761+
// CHECK-NEXT: apply [[FN]]<T, T>([[ARG0]], [[TMP]])
2762+
// CHECK-NEXT: destroy_addr [[ARG1]]
2763+
// CHECK-NEXT: destroy_addr [[TMP]]
2764+
// CHECK-NEXT: tuple
2765+
// CHECK-NEXT: dealloc_stack [[TMP]]
2766+
// CHECK-NEXT: return
2767+
sil @test_generic_partial_apply_apply : $@convention(thin) <T> (@in T, @in T) -> () {
2768+
bb0(%0 : $*T, %1 : $*T):
2769+
%f1 = function_ref @generic_callee : $@convention(thin) <T, U> (@in T, @in U) -> ()
2770+
%pa = partial_apply %f1<T, T>(%1) : $@convention(thin) <T, U> (@in T, @in U) -> ()
2771+
%a1 = apply %pa(%0) : $@callee_owned (@in T) -> ()
2772+
%r = tuple ()
2773+
return %r : $()
2774+
}
2775+
2776+
// CHECK-LABEL: sil @test_existential_partial_apply_apply
2777+
// CHECK: bb0(%0 : $*FakeProtocol):
2778+
// CHECK-NEXT: [[OPEN:%.*]] = open_existential_addr
2779+
// CHECK-NEXT: [[FN:%.*]] = witness_method
2780+
// CHECK-NEXT: apply [[FN]]<@opened("5DD6F3D0-808A-11E6-93A0-34363BD08DA0") FakeProtocol>([[OPEN]])
2781+
// CHECK-NEXT: tuple
2782+
// CHECK-NEXT: return
2783+
sil @test_existential_partial_apply_apply : $@convention(thin) (@in FakeProtocol) -> () {
2784+
bb0(%0: $*FakeProtocol):
2785+
%o = open_existential_addr %0 : $*FakeProtocol to $*@opened("5DD6F3D0-808A-11E6-93A0-34363BD08DA0") FakeProtocol
2786+
%f1 = witness_method $@opened("5DD6F3D0-808A-11E6-93A0-34363BD08DA0") FakeProtocol, #FakeProtocol.requirement!1, %o : $*@opened("5DD6F3D0-808A-11E6-93A0-34363BD08DA0") FakeProtocol : $@convention(witness_method) <T where T : FakeProtocol> (@in_guaranteed T) -> ()
2787+
%pa = partial_apply %f1<@opened("5DD6F3D0-808A-11E6-93A0-34363BD08DA0") FakeProtocol>() : $@convention(witness_method) <T where T : FakeProtocol> (@in_guaranteed T) -> ()
2788+
%a1 = apply %pa(%o) : $@callee_owned (@in_guaranteed @opened("5DD6F3D0-808A-11E6-93A0-34363BD08DA0") FakeProtocol) -> ()
2789+
2790+
%r = tuple ()
27492791
return %r : $()
27502792
}
27512793

27522794
sil @callee : $@convention(thin) (Double, @in_guaranteed Int) -> ()
2795+
sil @generic_callee : $@convention(thin) <T, U> (@in T, @in U) -> ()
27532796
sil @noreturn_func : $@convention(thin) () -> Never
27542797

27552798
// CHECK-LABEL: sil @remove_destroy_array

0 commit comments

Comments
 (0)