Skip to content

[4.2] Fix a SIL optimizer verification failure complaining about a not up-to-date dominator tree. #17056

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions lib/SILOptimizer/IPO/ClosureSpecializer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -784,7 +784,7 @@ void ClosureSpecCloner::populateCloned() {
namespace {

class SILClosureSpecializerTransform : public SILFunctionTransform {
void gatherCallSites(SILFunction *Caller,
bool gatherCallSites(SILFunction *Caller,
llvm::SmallVectorImpl<ClosureInfo*> &ClosureCandidates,
llvm::DenseSet<FullApplySite> &MultipleClosureAI);
bool specialize(SILFunction *Caller,
Expand Down Expand Up @@ -839,7 +839,7 @@ void SILClosureSpecializerTransform::run() {
invalidateAnalysis(SILAnalysis::InvalidationKind::Everything);
}

void SILClosureSpecializerTransform::gatherCallSites(
bool SILClosureSpecializerTransform::gatherCallSites(
SILFunction *Caller,
llvm::SmallVectorImpl<ClosureInfo*> &ClosureCandidates,
llvm::DenseSet<FullApplySite> &MultipleClosureAI) {
Expand All @@ -848,6 +848,8 @@ void SILClosureSpecializerTransform::gatherCallSites(
// make sure that we do not handle call sites with multiple closure arguments.
llvm::DenseSet<FullApplySite> VisitedAI;

bool CFGChanged = false;

// For each basic block BB in Caller...
for (auto &BB : *Caller) {

Expand Down Expand Up @@ -918,7 +920,7 @@ void SILClosureSpecializerTransform::gatherCallSites(
// We could fix this by inserting new arguments more carefully, or
// changing how we model dynamic Self altogether.
if (mayBindDynamicSelf(ApplyCallee))
return;
return CFGChanged;

// Ok, we know that we can perform the optimization but not whether or
// not the optimization is profitable. Find the index of the argument
Expand Down Expand Up @@ -991,12 +993,15 @@ void SILClosureSpecializerTransform::gatherCallSites(
}
if (CInfo) {
ValueLifetimeAnalysis VLA(CInfo->Closure, UsePoints);
VLA.computeFrontier(CInfo->LifetimeFrontier,
ValueLifetimeAnalysis::AllowToModifyCFG);
if (!VLA.computeFrontier(CInfo->LifetimeFrontier,
ValueLifetimeAnalysis::AllowToModifyCFG)) {
CFGChanged = true;
}
ClosureCandidates.push_back(CInfo);
}
}
}
return CFGChanged;
}

bool SILClosureSpecializerTransform::specialize(SILFunction *Caller,
Expand All @@ -1008,7 +1013,9 @@ bool SILClosureSpecializerTransform::specialize(SILFunction *Caller,
// ApplyInsts. Check the profitability of specializing the closure argument.
llvm::SmallVector<ClosureInfo*, 8> ClosureCandidates;
llvm::DenseSet<FullApplySite> MultipleClosureAI;
gatherCallSites(Caller, ClosureCandidates, MultipleClosureAI);
if (gatherCallSites(Caller, ClosureCandidates, MultipleClosureAI)) {
invalidateAnalysis(SILAnalysis::InvalidationKind::Branches);
}

bool Changed = false;
for (auto *CInfo : ClosureCandidates) {
Expand Down
49 changes: 49 additions & 0 deletions test/SILOptimizer/closure_specialize_and_cfg.sil
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// RUN: %target-sil-opt -assume-parsing-unqualified-ownership-sil -sil-verify-without-invalidation -enable-sil-verify-all -loop-unroll -closure-specialize %s

// Test if the ClosureSpecializer correctly invalidates the dominator tree
// even if there are no functions specialized.
// The test just checks if the compiler does not crash.
// First running LoopUnroll creates the dominator tree, which should then be
// invalidated by the ClosureSpecializer.
// If this is not done correctly the verification will complain that the
// dominator tree is not up to date.

import Builtin
import Swift

sil @closure : $@convention(thin) () -> ()

sil @use_closure : $@convention(thin) (@owned @callee_owned () -> ()) -> ()

sil hidden [noinline] @use_closure2 : $@convention(thin) (@owned @callee_owned () -> (), @owned @callee_owned () -> ()) -> () {
bb0(%0 : $@callee_owned () -> (), %1 : $@callee_owned () -> ()):
%2 = apply %0() : $@callee_owned () -> ()
%3 = apply %1() : $@callee_owned () -> ()
%4 = tuple ()
return %3 : $()
}

sil @insert_release_in_liferange_exit_block : $@convention(thin) () -> () {
bb0:
%2 = function_ref @closure : $@convention(thin) () -> ()
%3 = partial_apply %2() : $@convention(thin) () -> ()
%8 = function_ref @use_closure : $@convention(thin) (@owned @callee_owned () -> ()) -> ()
%5 = partial_apply %8(%3) : $@convention(thin) (@owned @callee_owned () -> ()) -> ()

// There is a critical edge from bb0 to bb2 which is broken by ValueLifetimeAnalysis.
cond_br undef, bb2, bb1

bb1:
strong_retain %3 : $@callee_owned () -> ()
strong_retain %3 : $@callee_owned () -> ()
%10 = function_ref @use_closure2 : $@convention(thin) (@owned @callee_owned () -> (), @owned @callee_owned () -> ()) -> ()

// Passing two closures actually prevents closure specialization.
%17 = apply %10(%3, %3) : $@convention(thin) (@owned @callee_owned () -> (), @owned @callee_owned () -> ()) -> ()
br bb2

bb2:
strong_release %5 : $@callee_owned () -> ()
%11 = tuple ()
return %11 : $()
}