Skip to content

Commit 349acca

Browse files
meg-guptameg-gupta
committed
Disable a few mark_dependence sil combines in ossa
There are 2 reasons to disable this: 1. OSSA rauw creates copies for replacement, copies created in this case cannot be canonicalized away due to the presence of pointer escape. (See test test_mark_dependence_ossa_silcombine1 after -sil-combine -copy-propagation) 2. SILCombine of mark_dependence (enum can cause a potential miscompile. ClosureLifetimeFixup specifically inserts enums to lifetime extend the closure lifetime until the end of the function. Replacing mark_dependence's base operand with a copy of the enum's operand can end up shortening the lifetime of the base. (See test test_mark_dependence_ossa_silcombine2 after -sil-combine -dce -semantic-arc-opts)
1 parent f177be0 commit 349acca

File tree

2 files changed

+95
-65
lines changed

2 files changed

+95
-65
lines changed

‎lib/SILOptimizer/SILCombiner/SILCombinerMiscVisitors.cpp‎

Lines changed: 30 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -1875,49 +1875,37 @@ static bool isLiteral(SILValue val) {
18751875
}
18761876

18771877
SILInstruction *SILCombiner::visitMarkDependenceInst(MarkDependenceInst *mdi) {
1878-
auto base = lookThroughOwnershipInsts(mdi->getBase());
1879-
1880-
// Simplify the base operand of a MarkDependenceInst to eliminate unnecessary
1881-
// instructions that aren't adding value.
1882-
//
1883-
// Conversions to Optional.Some(x) often happen here, this isn't important
1884-
// for us, we can just depend on 'x' directly.
1885-
if (auto *eiBase = dyn_cast<EnumInst>(base)) {
1886-
if (eiBase->hasOperand()) {
1887-
auto *use = &mdi->getOperandRef(MarkDependenceInst::Base);
1888-
OwnershipReplaceSingleUseHelper helper(ownershipFixupContext,
1889-
use, eiBase->getOperand());
1890-
if (helper) {
1891-
helper.perform();
1892-
tryEliminateOnlyOwnershipUsedForwardingInst(eiBase,
1893-
getInstModCallbacks());
1878+
if (!mdi->getFunction()->hasOwnership()) {
1879+
// Simplify the base operand of a MarkDependenceInst to eliminate
1880+
// unnecessary instructions that aren't adding value.
1881+
//
1882+
// Conversions to Optional.Some(x) often happen here, this isn't important
1883+
// for us, we can just depend on 'x' directly.
1884+
if (auto *eiBase = dyn_cast<EnumInst>(mdi->getBase())) {
1885+
if (eiBase->hasOperand()) {
1886+
mdi->setBase(eiBase->getOperand());
1887+
if (eiBase->use_empty()) {
1888+
eraseInstFromFunction(*eiBase);
1889+
}
18941890
return mdi;
18951891
}
18961892
}
1897-
}
18981893

1899-
// Conversions from a class to AnyObject also happen a lot, we can just depend
1900-
// on the class reference.
1901-
if (auto *ier = dyn_cast<InitExistentialRefInst>(base)) {
1902-
auto *use = &mdi->getOperandRef(MarkDependenceInst::Base);
1903-
OwnershipReplaceSingleUseHelper helper(ownershipFixupContext,
1904-
use, ier->getOperand());
1905-
if (helper) {
1906-
helper.perform();
1907-
tryEliminateOnlyOwnershipUsedForwardingInst(ier, getInstModCallbacks());
1894+
// Conversions from a class to AnyObject also happen a lot, we can just
1895+
// depend on the class reference.
1896+
if (auto *ier = dyn_cast<InitExistentialRefInst>(mdi->getBase())) {
1897+
mdi->setBase(ier->getOperand());
1898+
if (ier->use_empty())
1899+
eraseInstFromFunction(*ier);
19081900
return mdi;
19091901
}
1910-
}
19111902

1912-
// Conversions from a class to AnyObject also happen a lot, we can just depend
1913-
// on the class reference.
1914-
if (auto *oeri = dyn_cast<OpenExistentialRefInst>(base)) {
1915-
auto *use = &mdi->getOperandRef(MarkDependenceInst::Base);
1916-
OwnershipReplaceSingleUseHelper helper(ownershipFixupContext,
1917-
use, oeri->getOperand());
1918-
if (helper) {
1919-
helper.perform();
1920-
tryEliminateOnlyOwnershipUsedForwardingInst(oeri, getInstModCallbacks());
1903+
// Conversions from a class to AnyObject also happen a lot, we can just
1904+
// depend on the class reference.
1905+
if (auto *oeri = dyn_cast<OpenExistentialRefInst>(mdi->getBase())) {
1906+
mdi->setBase(oeri->getOperand());
1907+
if (oeri->use_empty())
1908+
eraseInstFromFunction(*oeri);
19211909
return mdi;
19221910
}
19231911
}
@@ -1926,17 +1914,14 @@ SILInstruction *SILCombiner::visitMarkDependenceInst(MarkDependenceInst *mdi) {
19261914
// whose base is a trivial typed object. In such a case, the mark_dependence
19271915
// does not have a meaning, so just eliminate it.
19281916
{
1929-
SILType baseType = base->getType();
1930-
if (baseType.isObject()) {
1931-
if ((hasOwnership() && base->getOwnershipKind() == OwnershipKind::None) ||
1932-
baseType.isTrivial(*mdi->getFunction())) {
1933-
SILValue value = mdi->getValue();
1934-
replaceInstUsesWith(*mdi, value);
1935-
return eraseInstFromFunction(*mdi);
1936-
}
1917+
SILType baseType = mdi->getBase()->getType();
1918+
if (baseType.isObject() && baseType.isTrivial(*mdi->getFunction())) {
1919+
SILValue value = mdi->getValue();
1920+
mdi->replaceAllUsesWith(value);
1921+
return eraseInstFromFunction(*mdi);
19371922
}
19381923
}
1939-
1924+
19401925
if (isLiteral(mdi->getValue())) {
19411926
// A literal lives forever, so no mark_dependence is needed.
19421927
// This pattern can occur after StringOptimization when a utf8CString of

‎test/SILOptimizer/sil_combine_ossa.sil‎

Lines changed: 65 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -4303,14 +4303,13 @@ bb2(%5 : @owned $MyErrorType):
43034303
throw %5 : $MyErrorType
43044304
}
43054305

4306-
// CHECK-LABEL: sil [ossa] @mark_dependence_base :
4307-
// CHECK: bb0(
4308-
// CHECK-NOT: init_existential_ref
4309-
// CHECK-NOT: enum
4310-
// CHECK: mark_dependence %0 : $*Builtin.Int64 on {{%.*}} : $B
4311-
// CHECK-NOT: init_existential_ref
4312-
// CHECK-NOT: enum
4313-
// CHECK: } // end sil function 'mark_dependence_base'
4306+
// CHECK-LABEL: sil [ossa] @mark_dependence_base
4307+
// XHECK: bb0(
4308+
// XHECK-NOT: init_existential_ref
4309+
// XHECK-NOT: enum
4310+
// XHECK-NEXT: mark_dependence
4311+
// XHECK-NEXT: load
4312+
// XHECK: return
43144313
sil [ossa] @mark_dependence_base : $@convention(thin) (@inout Builtin.Int64, @owned B) -> Builtin.Int64 {
43154314
bb0(%0 : $*Builtin.Int64, %1 : @owned $B):
43164315
%x = init_existential_ref %1 : $B : $B, $AnyObject
@@ -4322,10 +4321,10 @@ bb0(%0 : $*Builtin.Int64, %1 : @owned $B):
43224321
}
43234322

43244323
// CHECK-LABEL: sil [ossa] @mark_dependence_trivial_object_base :
4325-
// CHECK: bb0(
4326-
// CHECK-NEXT: copy_value
4327-
// CHECK-NEXT: return
4328-
// CHECK: } // end sil function 'mark_dependence_trivial_object_base'
4324+
// XHECK: bb0(
4325+
// XHECK-NEXT: copy_value
4326+
// XHECK-NEXT: return
4327+
// XHECK: } // end sil function 'mark_dependence_trivial_object_base'
43294328
sil [ossa] @mark_dependence_trivial_object_base : $@convention(thin) (@guaranteed B) -> @owned B {
43304329
bb0(%0 : @guaranteed $B):
43314330
%0a = copy_value %0 : $B
@@ -4357,14 +4356,13 @@ bb0(%addr : $*Builtin.Int64, %instance : @owned $B):
43574356

43584357
protocol _NSArrayCore {}
43594358

4360-
// CHECK-LABEL: sil [ossa] @mark_dependence_base2 :
4361-
// CHECK: bb0(
4362-
// CHECK-NOT: open_existential_ref
4363-
// CHECK-NOT: enum
4364-
// CHECK: mark_dependence %0 : $*Builtin.Int64 on {{%.*}} : $B
4365-
// CHECK-NOT: open_existential_ref
4366-
// CHECK-NOT: enum
4367-
// CHECK: } // end sil function 'mark_dependence_base2'
4359+
// CHECK-LABEL: sil [ossa] @mark_dependence_base2
4360+
// XHECK: bb0(
4361+
// XHECK-NOT: open_existential_ref
4362+
// XHECK-NOT: enum
4363+
// XHECK-NEXT: mark_dependence
4364+
// XHECK-NEXT: load
4365+
// XHECK: return
43684366
sil [ossa] @mark_dependence_base2 : $@convention(thin) (@inout Builtin.Int64, @owned B) -> Builtin.Int64 {
43694367
bb0(%0 : $*Builtin.Int64, %1 : @owned $B):
43704368
%2 = init_existential_ref %1 : $B : $B, $AnyObject
@@ -5416,3 +5414,50 @@ bb0(%0 : $*MoveOnlyStruct):
54165414
return %16 : $()
54175415
}
54185416

5417+
sil @originalClosure : $@convention(thin) (@owned Klass) -> ()
5418+
sil @useNoEscapeClosure : $@convention(thin) (@noescape @callee_guaranteed () -> ()) -> ()
5419+
5420+
// CHECK-LABEL: sil [ossa] @test_mark_dependence_ossa_silcombine1 :
5421+
// [[ENUM:%.*]] = enum
5422+
// [[MDI:%.*]] = mark_dependence {{.*}} on [[ENUM]]
5423+
// CHECK-LABEL: } // end sil function 'test_mark_dependence_ossa_silcombine1'
5424+
sil [ossa] @test_mark_dependence_ossa_silcombine1 : $@convention(thin) (@owned Klass) -> () {
5425+
bb0(%0 : @owned $Klass):
5426+
%1 = function_ref @originalClosure : $@convention(thin) (@owned Klass) -> ()
5427+
%2 = partial_apply [callee_guaranteed] %1(%0) : $@convention(thin) (@owned Klass) -> ()
5428+
%3 = convert_escape_to_noescape %2 : $@callee_guaranteed () -> () to $@noescape @callee_guaranteed () -> ()
5429+
%4 = enum $Optional<@callee_guaranteed () -> ()>, #Optional.some!enumelt, %2 : $@callee_guaranteed () -> ()
5430+
%5 = begin_borrow %4 : $Optional<@callee_guaranteed () -> ()>
5431+
%6 = mark_dependence %3 : $@noescape @callee_guaranteed () -> () on %5 : $Optional<@callee_guaranteed () -> ()>
5432+
%7 = function_ref @useNoEscapeClosure : $@convention(thin) (@noescape @callee_guaranteed () -> ()) -> ()
5433+
%8 = apply %7(%6) : $@convention(thin) (@noescape @callee_guaranteed () -> ()) -> ()
5434+
destroy_value %6 : $@noescape @callee_guaranteed () -> ()
5435+
end_borrow %5 : $Optional<@callee_guaranteed () -> ()>
5436+
destroy_value %4 : $Optional<@callee_guaranteed () -> ()>
5437+
%12 = tuple ()
5438+
return %12 : $()
5439+
}
5440+
5441+
// CHECK-LABEL: sil [ossa] @test_mark_dependence_ossa_silcombine2 :
5442+
// [[ENUM:%.*]] = enum
5443+
// [[MDI:%.*]] = mark_dependence {{.*}} on [[ENUM]]
5444+
// CHECK-LABEL: } // end sil function 'test_mark_dependence_ossa_silcombine2'
5445+
sil [ossa] @test_mark_dependence_ossa_silcombine2 : $@convention(thin) (@owned Klass) -> () {
5446+
bb0(%0 : @owned $Klass):
5447+
%1 = function_ref @originalClosure : $@convention(thin) (@owned Klass) -> ()
5448+
%2 = partial_apply [callee_guaranteed] %1(%0) : $@convention(thin) (@owned Klass) -> ()
5449+
%3 = convert_escape_to_noescape %2 : $@callee_guaranteed () -> () to $@noescape @callee_guaranteed () -> ()
5450+
%4 = enum $Optional<@callee_guaranteed () -> ()>, #Optional.some!enumelt, %2 : $@callee_guaranteed () -> ()
5451+
%5 = begin_borrow %4 : $Optional<@callee_guaranteed () -> ()>
5452+
%6 = mark_dependence %3 : $@noescape @callee_guaranteed () -> () on %5 : $Optional<@callee_guaranteed () -> ()>
5453+
%7 = function_ref @useNoEscapeClosure : $@convention(thin) (@noescape @callee_guaranteed () -> ()) -> ()
5454+
%8 = apply %7(%6) : $@convention(thin) (@noescape @callee_guaranteed () -> ()) -> ()
5455+
destroy_value %6 : $@noescape @callee_guaranteed () -> ()
5456+
br bb1(%4 : $Optional<@callee_guaranteed () -> ()>, %5 : $Optional<@callee_guaranteed () -> ()>)
5457+
5458+
bb1(%10 : @owned $Optional<@callee_guaranteed () -> ()>, %11 : @guaranteed $Optional<@callee_guaranteed () -> ()>):
5459+
end_borrow %11 : $Optional<@callee_guaranteed () -> ()>
5460+
destroy_value %10 : $Optional<@callee_guaranteed () -> ()>
5461+
%12 = tuple ()
5462+
return %12 : $()
5463+
}

0 commit comments

Comments
 (0)