Skip to content

[ownership] Convert ValueOwnershipKind to have an Invalid state instead of using optional. #34659

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
31 changes: 19 additions & 12 deletions include/swift/SIL/SILValue.h
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,10 @@ llvm::raw_ostream &operator<<(llvm::raw_ostream &os,
/// have.
struct ValueOwnershipKind {
enum innerty : uint8_t {
/// A value used to signal that two merged ValueOwnershipKinds were
/// incompatible.
Invalid = 0,

/// A SILValue with `Unowned` ownership kind is an independent value that
/// has a lifetime that is only guaranteed to last until the next program
/// visible side-effect. To maintain the lifetime of an unowned value, it
Expand Down Expand Up @@ -156,7 +160,10 @@ struct ValueOwnershipKind {
return Value == b;
}

Optional<ValueOwnershipKind> merge(ValueOwnershipKind RHS) const;
/// Returns true if this ValueOwnershipKind is not invalid.
explicit operator bool() const { return Value != Invalid; }

ValueOwnershipKind merge(ValueOwnershipKind RHS) const;

/// Given that there is an aggregate value (like a struct or enum) with this
/// ownership kind, and a subobject of type Proj is being projected from the
Expand All @@ -172,6 +179,8 @@ struct ValueOwnershipKind {
/// kinds.
UseLifetimeConstraint getForwardingLifetimeConstraint() const {
switch (Value) {
case ValueOwnershipKind::Invalid:
llvm_unreachable("Invalid ownership doesnt have a lifetime constraint!");
case ValueOwnershipKind::None:
case ValueOwnershipKind::Guaranteed:
case ValueOwnershipKind::Unowned:
Expand All @@ -188,7 +197,7 @@ struct ValueOwnershipKind {
/// The reason why we do not compare directy is to allow for
/// ValueOwnershipKind::None to merge into other forms of ValueOwnershipKind.
bool isCompatibleWith(ValueOwnershipKind other) const {
return merge(other).hasValue();
return bool(merge(other));
}

/// Returns isCompatibleWith(other.getOwnershipKind()).
Expand All @@ -197,16 +206,14 @@ struct ValueOwnershipKind {
/// dependencies.
bool isCompatibleWith(SILValue other) const;

template <typename RangeTy>
static Optional<ValueOwnershipKind> merge(RangeTy &&r) {
auto initial = Optional<ValueOwnershipKind>(ValueOwnershipKind::None);
return accumulate(
std::forward<RangeTy>(r), initial,
[](Optional<ValueOwnershipKind> acc, ValueOwnershipKind x) {
if (!acc)
return acc;
return acc.getValue().merge(x);
});
template <typename RangeTy> static ValueOwnershipKind merge(RangeTy &&r) {
auto initial = ValueOwnershipKind::None;
return accumulate(std::forward<RangeTy>(r), initial,
[](ValueOwnershipKind acc, ValueOwnershipKind x) {
if (!acc)
return acc;
return acc.merge(x);
});
}

StringRef asString() const;
Expand Down
5 changes: 4 additions & 1 deletion lib/SIL/IR/SILInstructions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2886,5 +2886,8 @@ ReturnInst::ReturnInst(SILFunction &func, SILDebugLocation debugLoc,
});

// Then merge all of our ownership kinds. Assert if we fail to merge.
ownershipKind = *ValueOwnershipKind::merge(ownershipKindRange);
ownershipKind = ValueOwnershipKind::merge(ownershipKindRange);
assert(ownershipKind != ValueOwnershipKind::Invalid &&
"Conflicting ownership kinds when creating term inst from function "
"result info?!");
}
34 changes: 21 additions & 13 deletions lib/SIL/IR/SILValue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,8 @@ ValueOwnershipKind::ValueOwnershipKind(const SILFunction &F, SILType Type,

StringRef ValueOwnershipKind::asString() const {
switch (Value) {
case ValueOwnershipKind::Invalid:
return "invalid";
case ValueOwnershipKind::Unowned:
return "unowned";
case ValueOwnershipKind::Owned:
Expand All @@ -215,21 +217,27 @@ llvm::raw_ostream &swift::operator<<(llvm::raw_ostream &os,
return os << kind.asString();
}

Optional<ValueOwnershipKind>
ValueOwnershipKind::merge(ValueOwnershipKind RHS) const {
auto LHSVal = Value;
auto RHSVal = RHS.Value;
ValueOwnershipKind ValueOwnershipKind::merge(ValueOwnershipKind rhs) const {
auto lhsVal = Value;
auto rhsVal = rhs.Value;

// Any merges with anything.
if (LHSVal == ValueOwnershipKind::None) {
return ValueOwnershipKind(RHSVal);
}
// Any merges with anything.
if (RHSVal == ValueOwnershipKind::None) {
return ValueOwnershipKind(LHSVal);
}
// If either lhs or rhs are invalid, return invalid.
if (lhsVal == ValueOwnershipKind::Invalid ||
rhsVal == ValueOwnershipKind::Invalid)
return ValueOwnershipKind::Invalid;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You shouldn't be special-casing invalid here.


// None merges with anything.
if (lhsVal == ValueOwnershipKind::None)
return ValueOwnershipKind(rhsVal);
if (rhsVal == ValueOwnershipKind::None)
return ValueOwnershipKind(lhsVal);

return (LHSVal == RHSVal) ? Optional<ValueOwnershipKind>(*this) : llvm::None;
// At this point, if the two ownership kinds don't line up, the merge fails.
if (lhsVal != rhsVal)
return ValueOwnershipKind::Invalid;

// Otherwise, we are good, return *this.
return *this;
}

ValueOwnershipKind::ValueOwnershipKind(StringRef S) {
Expand Down
6 changes: 3 additions & 3 deletions lib/SIL/IR/ValueOwnership.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ ValueOwnershipKindClassifier::visitForwardingInst(SILInstruction *i,
return op.get().getOwnershipKind();
}));

if (!mergedValue.hasValue()) {
if (!mergedValue) {
// If we have mismatched SILOwnership and sil ownership is not enabled,
// just return None for staging purposes. If SILOwnership is enabled, then
// we must assert!
Expand All @@ -235,7 +235,7 @@ ValueOwnershipKindClassifier::visitForwardingInst(SILInstruction *i,
llvm_unreachable("Forwarding inst with mismatching ownership kinds?!");
}

return mergedValue.getValue();
return mergedValue;
}

#define FORWARDING_OWNERSHIP_INST(INST) \
Expand Down Expand Up @@ -341,7 +341,7 @@ ValueOwnershipKind ValueOwnershipKindClassifier::visitApplyInst(ApplyInst *ai) {
llvm_unreachable("Forwarding inst with mismatching ownership kinds?!");
}

return *mergedOwnershipKind;
return mergedOwnershipKind;
}

ValueOwnershipKind ValueOwnershipKindClassifier::visitLoadInst(LoadInst *LI) {
Expand Down
4 changes: 4 additions & 0 deletions lib/SIL/Verifier/SILOwnershipVerifier.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -487,6 +487,8 @@ bool SILValueOwnershipChecker::gatherUsers(
bool SILValueOwnershipChecker::checkFunctionArgWithoutLifetimeEndingUses(
SILFunctionArgument *arg) {
switch (arg->getOwnershipKind()) {
case ValueOwnershipKind::Invalid:
llvm_unreachable("Invalid ownership kind used?!");
case ValueOwnershipKind::Guaranteed:
case ValueOwnershipKind::Unowned:
case ValueOwnershipKind::None:
Expand All @@ -507,6 +509,8 @@ bool SILValueOwnershipChecker::checkFunctionArgWithoutLifetimeEndingUses(
bool SILValueOwnershipChecker::checkYieldWithoutLifetimeEndingUses(
BeginApplyResult *yield, ArrayRef<Operand *> regularUses) {
switch (yield->getOwnershipKind()) {
case ValueOwnershipKind::Invalid:
llvm_unreachable("Invalid ownership kind used?!");
case ValueOwnershipKind::Unowned:
case ValueOwnershipKind::None:
return true;
Expand Down
6 changes: 3 additions & 3 deletions lib/SILGen/RValue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -388,7 +388,7 @@ static void verifyHelper(ArrayRef<ManagedValue> values,
NullablePtr<SILGenFunction> SGF = nullptr) {
// This is a no-op in non-assert builds.
#ifndef NDEBUG
auto result = Optional<ValueOwnershipKind>(ValueOwnershipKind::None);
ValueOwnershipKind result = ValueOwnershipKind::None;
Optional<bool> sameHaveCleanups;
for (ManagedValue v : values) {
assert((!SGF || !v.getType().isLoadable(SGF.get()->F) ||
Expand All @@ -408,8 +408,8 @@ static void verifyHelper(ArrayRef<ManagedValue> values,

// This variable is here so that if the assert below fires, the current
// reduction value is still available.
auto newResult = result.getValue().merge(kind);
assert(newResult.hasValue());
auto newResult = result.merge(kind);
assert(newResult);
result = newResult;
}
#endif
Expand Down
2 changes: 2 additions & 0 deletions lib/SILOptimizer/Differentiation/Thunk.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -496,6 +496,8 @@ SILFunction *getOrCreateReabstractionThunk(SILOptFunctionBuilder &fb,
// Owned values need to be destroyed.
for (auto arg : valuesToCleanup) {
switch (arg.getOwnershipKind()) {
case ValueOwnershipKind::Invalid:
llvm_unreachable("Invalid ownership kind?!");
case ValueOwnershipKind::Guaranteed:
builder.emitEndBorrowOperation(loc, arg);
break;
Expand Down
4 changes: 4 additions & 0 deletions test/SIL/ownership-verifier/undef.sil
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ struct MyInt {
// CHECK-NEXT: Operand Ownership Map:
// CHECK-NEXT: Op #: 0
// CHECK-NEXT: Ownership Map: -- OperandOwnershipKindMap --
// CHECK-NEXT: invalid: No.
// CHECK-NEXT: unowned: No.
// CHECK-NEXT: owned: Yes. Liveness: LifetimeEnding
// CHECK-NEXT: guaranteed: No.
Expand All @@ -28,6 +29,7 @@ struct MyInt {
// CHECK-NEXT: Operand Ownership Map:
// CHECK-NEXT: Op #: 0
// CHECK-NEXT: Ownership Map: -- OperandOwnershipKindMap --
// CHECK-NEXT: invalid: No.
// CHECK-NEXT: unowned: No.
// CHECK-NEXT: owned: Yes. Liveness: LifetimeEnding
// CHECK-NEXT: guaranteed: No.
Expand All @@ -36,6 +38,7 @@ struct MyInt {
// CHECK-NEXT: Operand Ownership Map:
// CHECK-NEXT: Op #: 0
// CHECK-NEXT: Ownership Map: -- OperandOwnershipKindMap --
// CHECK-NEXT: invalid: No.
// CHECK-NEXT: unowned: No.
// CHECK-NEXT: owned: Yes. Liveness: LifetimeEnding
// CHECK-NEXT: guaranteed: No.
Expand All @@ -44,6 +47,7 @@ struct MyInt {
// CHECK-NEXT: Operand Ownership Map:
// CHECK-NEXT: Op #: 0
// CHECK-NEXT: Ownership Map: -- OperandOwnershipKindMap --
// CHECK-NEXT: invalid: No.
// CHECK-NEXT: unowned: No.
// CHECK-NEXT: owned: No.
// CHECK-NEXT: guaranteed: No.
Expand Down