Skip to content

Commit 58d4191

Browse files
committed
[ownership] Try harder to make sure we do not propagate ownership info when ownership is disabled.
Specifically, I made it so that assuming our instruction is inserted into a block already that we: 1. Return a constraint of {OwnershipKind::Any, UseLifetimeConstraint::NonLifetimeEnding}. 2. Return OwnershipKind::None for all values. Noticed above I said that if the instruction is already inserted into a block then we do this. The reason why is that if this is called before an instruction is inserted into a block, we can't get access to the SILFunction that has the information on whether or not we are in OSSA form. The only time this can happen is if one is using these APIs from within SILBuilder since SILBuilder is the only place where we allow this to happen. In SILBuilder, we already know whether or not our function is in ossa or not and already does different things as appropriate (namely in non-ossa does not call getOwnershipKind()). So we know that if these APIs are called in such a situation, we will only be calling it if we are in OSSA already. Given that, we just assume we are in OSSA if we do not have a function. To make sure that no mistakes are made as a result of that assumption, I put in a verifier check that all values when ownership is disabled return a OwnershipKind::None from getOwnershipKind(). The main upside to this is this means that we can write code for both OSSA/non-OSSA and write code for non-None ownership without needing to check if ownership is enabled.
1 parent cd9218c commit 58d4191

File tree

6 files changed

+59
-9
lines changed

6 files changed

+59
-9
lines changed

lib/SIL/IR/OperandOwnership.cpp

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -913,6 +913,19 @@ OwnershipConstraintClassifier::visitBuiltinInst(BuiltinInst *bi) {
913913
Optional<OwnershipConstraint> Operand::getOwnershipConstraint() const {
914914
if (isTypeDependent())
915915
return None;
916+
917+
// If we do not have ownership enabled, just return any. This ensures that we
918+
// do not have any consuming uses and everything from an ownership perspective
919+
// is just a liveness use short-circuiting many of the optimizations.
920+
//
921+
// We do not ever call this function when an instruction isn't in a block.
922+
assert(getUser()->getParent() &&
923+
"Can not lookup ownership constraint unless inserted into block");
924+
if (auto *block = getUser()->getParent())
925+
if (auto *func = block->getParent())
926+
if (!func->hasOwnership())
927+
return {{OwnershipKind::Any, UseLifetimeConstraint::NonLifetimeEnding}};
928+
916929
OwnershipConstraintClassifier classifier(getUser()->getModule(), *this);
917930
return classifier.visit(const_cast<SILInstruction *>(getUser()));
918931
}

lib/SIL/IR/SILUndef.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
using namespace swift;
1717

1818
static ValueOwnershipKind getOwnershipKindForUndef(SILType type, const SILFunction &f) {
19+
if (!f.hasOwnership())
20+
return OwnershipKind::None;
1921
if (type.isAddress() || type.isTrivial(f))
2022
return OwnershipKind::None;
2123
return OwnershipKind::Owned;

lib/SIL/IR/ValueOwnership.cpp

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -583,6 +583,19 @@ ValueOwnershipKindClassifier::visitBuiltinInst(BuiltinInst *BI) {
583583
//===----------------------------------------------------------------------===//
584584

585585
ValueOwnershipKind SILValue::getOwnershipKind() const {
586+
// If we do not have an undef, we should always be able to get to our function
587+
// here. If we do not have ownership enabled, just return none for everything
588+
// to short circuit ownership optimizations. If we have an undef we may still
589+
// get some results that are slightly wonky but hopefully when we lower
590+
// ownership we remove that.
591+
//
592+
// We assume that any time we are in SILBuilder and call this without having a
593+
// value in a block yet, ossa is enabled.
594+
if (auto *block = Value->getParentBlock())
595+
if (auto *f = block->getParent())
596+
if (!f->hasOwnership())
597+
return OwnershipKind::None;
598+
586599
ValueOwnershipKindClassifier Classifier;
587600
auto result = Classifier.visit(const_cast<ValueBase *>(Value));
588601
assert(result && "Returned ownership kind invalid on values");

lib/SIL/Verifier/SILVerifier.cpp

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1022,8 +1022,11 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
10221022
assert(F && "Expected value base with parent function");
10231023
// If we do not have qualified ownership, then do not verify value base
10241024
// ownership.
1025-
if (!F->hasOwnership())
1025+
if (!F->hasOwnership()) {
1026+
require(SILValue(V).getOwnershipKind() == OwnershipKind::None,
1027+
"Once ownership is gone, all values should have none ownership");
10261028
return;
1029+
}
10271030
SILValue(V).verifyOwnership(&DEBlocks);
10281031
}
10291032

@@ -1100,14 +1103,36 @@ class SILVerifier : public SILVerifierBase<SILVerifier> {
11001103
"instruction's operand's owner isn't the instruction");
11011104
require(isInValueUses(&operand), "operand value isn't used by operand");
11021105

1103-
if (I->isTypeDependentOperand(operand)) {
1106+
if (operand.isTypeDependent()) {
11041107
require(isa<SILInstruction>(I),
11051108
"opened archetype operand should refer to a SILInstruction");
11061109
}
11071110

11081111
// Make sure that if operand is generic that its primary archetypes match
11091112
// the function context.
11101113
checkLegalType(I->getFunction(), operand.get(), I);
1114+
1115+
// If we are not in OSSA, our operand constraint should be invalid for a
1116+
// type dependent operand (that is Optional::None) and if we have a non
1117+
// type dependent operand then we should have a constraint of
1118+
// OwnershipKind::Any, UseLifetimeConstraint::NonLifetimeEnding.
1119+
if (!I->getFunction()->hasOwnership()) {
1120+
if (operand.isTypeDependent()) {
1121+
require(
1122+
!operand.getOwnershipConstraint(),
1123+
"Non Optional::None constraint for a type dependent operand?!");
1124+
} else {
1125+
auto constraint = operand.getOwnershipConstraint();
1126+
require(constraint.hasValue(),
1127+
"All non-type dependent operands must have a "
1128+
"non-Optional::None constraint?!");
1129+
require(constraint->getPreferredKind() == OwnershipKind::Any &&
1130+
constraint->getLifetimeConstraint() ==
1131+
UseLifetimeConstraint::NonLifetimeEnding,
1132+
"In non-ossa all non-type dependent operands must have a "
1133+
"constraint of Any, NonLifetimeEnding");
1134+
}
1135+
}
11111136
}
11121137

11131138
// TODO: There should be a use of an opened archetype inside the instruction for

test/AutoDiff/SILOptimizer/differentiation_function_canonicalization.sil

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,7 @@ bb0(%0 : $@differentiable @callee_guaranteed (Float) -> Float):
4545
// CHECK: bb0([[ARG:%.*]] : $@differentiable @callee_guaranteed (Float) -> Float):
4646
// CHECK: [[ORIG_FN:%.*]] = differentiable_function_extract [original] [[ARG]]
4747
// CHECK: [[JVP_FN:%.*]] = differentiable_function_extract [jvp] [[ARG]]
48-
// CHECK: strong_retain [[JVP_FN]]
4948
// CHECK: [[VJP_FN:%.*]] = differentiable_function_extract [vjp] [[ARG]]
50-
// CHECK: strong_retain [[VJP_FN]]
5149
// CHECK: differentiable_function [parameters 0] [results 0] [[ORIG_FN]] : {{.*}} with_derivative {[[JVP_FN]] : {{.*}}, [[VJP_FN]] : {{.*}}}
5250
// CHECK: }
5351

@@ -134,8 +132,6 @@ bb0(%0 : $Class):
134132
// CHECK-LABEL: sil @test_class_method_partial_apply
135133
// CHECK: bb0([[ARG:%.*]] : $Class):
136134
// CHECK: [[ORIG_FN:%.*]] = class_method [[ARG]] : $Class, #Class.method
137-
// CHECK: strong_retain [[ARG]]
138-
// CHECK: strong_retain [[ARG]]
139135
// CHECK: [[ORIG_FN_PARTIALLY_APPLIED:%.*]] = partial_apply [callee_guaranteed] [[ORIG_FN]]([[ARG]])
140136
// CHECK: [[JVP_FN:%.*]] = class_method [[ARG]] : $Class, #Class.method!jvp.SU
141137
// CHECK: [[JVP_FN_PARTIALLY_APPLIED:%.*]] = partial_apply [callee_guaranteed] [[JVP_FN]]([[ARG]])

test/SIL/Parser/basic.sil

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1591,11 +1591,12 @@ bb0(%0 : $Builtin.NativeObject):
15911591
return undef : $()
15921592
}
15931593

1594-
// CHECK-LABEL: sil @test_unchecked_ownership_conversion : $@convention(thin) (@guaranteed Builtin.NativeObject) -> () {
1594+
// CHECK-LABEL: sil [ossa] @test_unchecked_ownership_conversion : $@convention(thin) (@guaranteed Builtin.NativeObject) -> () {
15951595
// CHECK: unchecked_ownership_conversion {{%.*}} : $Builtin.NativeObject, @guaranteed to @owned
1596-
sil @test_unchecked_ownership_conversion : $@convention(thin) (@guaranteed Builtin.NativeObject) -> () {
1597-
bb0(%0 : $Builtin.NativeObject):
1596+
sil [ossa] @test_unchecked_ownership_conversion : $@convention(thin) (@guaranteed Builtin.NativeObject) -> () {
1597+
bb0(%0 : @guaranteed $Builtin.NativeObject):
15981598
%1 = unchecked_ownership_conversion %0 : $Builtin.NativeObject, @guaranteed to @owned
1599+
end_lifetime %1 : $Builtin.NativeObject
15991600
return undef : $()
16001601
}
16011602

0 commit comments

Comments
 (0)