Skip to content

Fix CSE::processOpenExistentialRef #35265

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.

Sign up for GitHub

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 1 commit into from
Jan 6, 2021
Merged

Fix CSE::processOpenExistentialRef #35265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
26 changes: 9 additions & 17 deletions lib/SILOptimizer/Transforms/CSE.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -581,10 +581,6 @@ class CSE {

SILOptFunctionBuilder &FuncBuilder;

SILOpenedArchetypesTracker &OpenedArchetypesTracker;

InstructionCloner &Cloner;

DeadEndBlocks &DeadEndBBs;

OwnershipFixupContext &FixupCtx;
Expand All @@ -594,16 +590,10 @@ class CSE {
llvm::SmallVector<ApplyInst *, 8> lazyPropertyGetters;

CSE(bool RunsOnHighLevelSil, SideEffectAnalysis *SEA,
SILOptFunctionBuilder &FuncBuilder,
SILOpenedArchetypesTracker &OpenedArchetypesTracker,
InstructionCloner &Cloner, DeadEndBlocks &DeadEndBBs,
SILOptFunctionBuilder &FuncBuilder, DeadEndBlocks &DeadEndBBs,
OwnershipFixupContext &FixupCtx)
: SEA(SEA), FuncBuilder(FuncBuilder),
OpenedArchetypesTracker(OpenedArchetypesTracker), Cloner(Cloner),
DeadEndBBs(DeadEndBBs), FixupCtx(FixupCtx),
RunsOnHighLevelSil(RunsOnHighLevelSil) {
Cloner.getBuilder().setOpenedArchetypesTracker(&OpenedArchetypesTracker);
}
: SEA(SEA), FuncBuilder(FuncBuilder), DeadEndBBs(DeadEndBBs),
FixupCtx(FixupCtx), RunsOnHighLevelSil(RunsOnHighLevelSil) {}

bool processFunction(SILFunction &F, DominanceInfo *DT);

Expand Down Expand Up @@ -843,11 +833,16 @@ bool CSE::processOpenExistentialRef(OpenExistentialRefInst *Inst,
}

// Now process candidates.
SILOpenedArchetypesTracker OpenedArchetypesTracker(Inst->getFunction());
// Register the new archetype to be used.
OpenedArchetypesTracker.registerOpenedArchetypes(VI);
// Use a cloner. It makes copying the instruction and remapping of
// opened archetypes trivial.
InstructionCloner Cloner(Inst->getFunction());
Cloner.registerOpenedExistentialRemapping(
OldOpenedArchetype->castTo<ArchetypeType>(), NewOpenedArchetype);
auto &Builder = Cloner.getBuilder();
Builder.setOpenedArchetypesTracker(&OpenedArchetypesTracker);

llvm::SmallPtrSet<SILInstruction *, 16> Processed;
// Now clone each candidate and replace the opened archetype
Expand Down Expand Up @@ -1401,14 +1396,11 @@ class SILCSE : public SILFunctionTransform {
SILOptFunctionBuilder FuncBuilder(*this);

auto *Fn = getFunction();
SILOpenedArchetypesTracker OpenedArchetypesTracker(Fn);
InstructionCloner Cloner(Fn);
DeadEndBlocks DeadEndBBs(Fn);
JointPostDominanceSetComputer Computer(DeadEndBBs);
InstModCallbacks callbacks;
OwnershipFixupContext FixupCtx{callbacks, DeadEndBBs, Computer};
CSE C(RunsOnHighLevelSil, SEA, FuncBuilder, OpenedArchetypesTracker, Cloner,
DeadEndBBs, FixupCtx);
CSE C(RunsOnHighLevelSil, SEA, FuncBuilder, DeadEndBBs, FixupCtx);
bool Changed = false;

// Perform the traditional CSE.
Expand Down
26 changes: 26 additions & 0 deletions test/SILOptimizer/cse.sil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1244,6 +1244,32 @@ bb3:
return %20 : $()
}

// This test is not a repro but used to show why we need a local Cloner object in CSE::processOpenExistentialRef.
// Discussion:
// Since the CSE pass can delete instructions and allocate instructions whose state can be cached in the Cloner, we have to use local Cloner object so that we don't accidentally refer to stale state
// In this test users of %9 are all cloned and original instructions are deleted. Users of %13 are similarly cloned and original instructions are deleted so the types can be remapped.
// Instructions can further get deleted in the main CSE pass as well as during SimplifyInstruction.
// For some heap allocation patterns newly allocated instructions can get the same address of a previously allocated instruction, and can end up referring to a stale state in the Cloner.
//
// CHECK-LABEL: sil @cse_open_existential_ref_local_cloner :
// CHECK: open_existential_ref
// CHECK-NOT: open_existential_ref
// CHECK-LABEL: } // end sil function 'cse_open_existential_ref_local_cloner'
sil @cse_open_existential_ref_local_cloner : $@convention(thin) (@guaranteed Proto, Bool) -> () {
bb0(%0 : $Proto, %1 : $Bool):
%4 = open_existential_ref %0 : $Proto to $@opened("1B68354A-4796-11E6-B7DF-B8E856428C60") Proto
%5 = witness_method $@opened("1B68354A-4796-11E6-B7DF-B8E856428C60") Proto, #Proto.doThis, %4 : $@opened("1B68354A-4796-11E6-B7DF-B8E856428C60") Proto : $@convention(witness_method: Proto) <τ_0_0 where τ_0_0 : Proto> (@guaranteed τ_0_0) -> ()
%6 = apply %5<@opened("1B68354A-4796-11E6-B7DF-B8E856428C60") Proto>(%4) : $@convention(witness_method: Proto) <τ_0_0 where τ_0_0 : Proto> (@guaranteed τ_0_0) -> ()
%9 = open_existential_ref %0 : $Proto to $@opened("1B685052-4796-11E6-B7DF-B8E856428C60") Proto
%10 = witness_method $@opened("1B685052-4796-11E6-B7DF-B8E856428C60") Proto, #Proto.doThat, %9 : $@opened("1B685052-4796-11E6-B7DF-B8E856428C60") Proto : $@convention(witness_method: Proto) <τ_0_0 where τ_0_0 : Proto> (@guaranteed τ_0_0) -> ()
%11 = apply %10<@opened("1B685052-4796-11E6-B7DF-B8E856428C60") Proto>(%9) : $@convention(witness_method: Proto) <τ_0_0 where τ_0_0 : Proto> (@guaranteed τ_0_0) -> ()
%13 = open_existential_ref %0 : $Proto to $@opened("1B6851A6-4796-11E6-B7DF-B8E856428C60") Proto
%14 = witness_method $@opened("1B6851A6-4796-11E6-B7DF-B8E856428C60") Proto, #Proto.doThis, %13 : $@opened("1B6851A6-4796-11E6-B7DF-B8E856428C60") Proto : $@convention(witness_method: Proto) <τ_0_0 where τ_0_0 : Proto> (@guaranteed τ_0_0) -> ()
%15 = apply %14<@opened("1B6851A6-4796-11E6-B7DF-B8E856428C60") Proto>(%13) : $@convention(witness_method: Proto) <τ_0_0 where τ_0_0 : Proto> (@guaranteed τ_0_0) -> ()
%res = tuple ()
return %res : $()
}

// Check that we don't CSE open_existential_ref if they are not compeltely equal.
// CHECK-LABEL: sil @dont_cse_open_existential_ref
// CHECK: open_existential_ref
Expand Down