Skip to content

APIs for terminator results that forward ownership #62493

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 2 commits into from
Dec 13, 2022
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
32 changes: 14 additions & 18 deletions include/swift/SIL/SILArgument.h
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ class SILArgument : public ValueBase {
unsigned getIndex() const;

/// Return non-null if \p value is a phi.
static SILPhiArgument *isPhi(SILValue value);
static SILPhiArgument *asPhi(SILValue value);

/// Return non-null if \p value is a terminator result.
static SILPhiArgument *isTerminatorResult(SILValue value);
Expand All @@ -145,10 +145,15 @@ class SILArgument : public ValueBase {
/// If this argument is a phi, populate `OutArray` with the incoming phi
/// values for each predecessor BB. If this argument is not a phi, return
/// false.
///
/// If this block has no predecessors, returnedPhiValues will be empty.
bool getIncomingPhiValues(SmallVectorImpl<SILValue> &returnedPhiValues) const;

/// If this argument is a phi, populate `OutArray` with each predecessor block
/// and its incoming phi value. If this argument is not a phi, return false.
///
/// If this block has no predecessors, returnedPredAndPhiValuePairs will be
/// empty.
bool
getIncomingPhiValues(SmallVectorImpl<std::pair<SILBasicBlock *, SILValue>>
&returnedPredAndPhiValuePairs) const;
Expand Down Expand Up @@ -187,14 +192,6 @@ class SILArgument : public ValueBase {
SmallVectorImpl<std::pair<SILBasicBlock *, SILValue>>
&returnedSingleTermOperands) const;

/// If this SILArgument's parent block has a single predecessor whose
/// terminator has a single operand, return the incoming operand of the
/// predecessor's terminator. Returns SILValue() otherwise. Note that for
/// some predecessor terminators the incoming value is not exactly the
/// argument value. E.g. the incoming value for a switch_enum payload argument
/// is the enum itself (the operand of the switch_enum).
SILValue getSingleTerminatorOperand() const;

/// If this SILArgument's parent block has a single predecessor whose
/// terminator has a single operand, return that terminator.
TermInst *getSingleTerminator() const;
Expand All @@ -203,6 +200,13 @@ class SILArgument : public ValueBase {
/// otherwise return nullptr.
TermInst *getTerminatorForResult() const;

/// If this terminator result forwards an operand, then return it.
///
/// Precondition: this->isTerminatorResult()
///
/// TODO: Move this and other APIs into a TerminatorResult abstraction.
const Operand *forwardedTerminatorResultOperand() const;

/// Return the SILArgumentKind of this argument.
SILArgumentKind getKind() const {
return SILArgumentKind(ValueBase::getKind());
Expand Down Expand Up @@ -310,14 +314,6 @@ class SILPhiArgument : public SILArgument {
SmallVectorImpl<std::pair<SILBasicBlock *, SILValue>>
&returnedSingleTermOperands) const;

/// If this SILArgument's parent block has a single predecessor whose
/// terminator has a single operand, return the incoming operand of the
/// predecessor's terminator. Returns SILValue() otherwise. Note that for
/// some predecessor terminators the incoming value is not exactly the
/// argument value. E.g. the incoming value for a switch_enum payload argument
/// is the enum itself (the operand of the switch_enum).
SILValue getSingleTerminatorOperand() const;

/// If this SILArgument's parent block has a single predecessor whose
/// terminator has a single operand, return that terminator.
TermInst *getSingleTerminator() const;
Expand Down Expand Up @@ -406,7 +402,7 @@ class SILFunctionArgument : public SILArgument {
//===----------------------------------------------------------------------===//

/// Return non-null if \p value is a real phi argument.
inline SILPhiArgument *SILArgument::isPhi(SILValue value) {
inline SILPhiArgument *SILArgument::asPhi(SILValue value) {
if (auto *arg = dyn_cast<SILPhiArgument>(value)) {
if (arg->isPhi())
return arg;
Expand Down
26 changes: 15 additions & 11 deletions include/swift/SIL/SILBasicBlock.h
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,7 @@ public SwiftObjectHeader {
/// Whether the block has any phi arguments.
///
/// Note that a block could have an argument and still return false. The
/// argument must also satisfy SILPhiArgument::isPhiArgument.
/// argument must also satisfy SILPhiArgument::isPhi.
bool hasPhi() const;

//===--------------------------------------------------------------------===//
Expand Down Expand Up @@ -541,6 +541,8 @@ struct PhiOperand {

PhiOperand() = default;

// This if \p operand is a CondBrInst operand, then this constructs and
// invalid PhiOperand. The abstraction only works for non-trivial OSSA values.
PhiOperand(Operand *operand) {
auto *branch = dyn_cast<BranchInst>(operand->getUser());
if (!branch)
Expand All @@ -567,8 +569,8 @@ struct PhiOperand {
}

SILPhiArgument *getValue() const {
auto *branch = cast<BranchInst>(predBlock->getTerminator());
return cast<SILPhiArgument>(branch->getDestBB()->getArgument(argIndex));
return
cast<SILPhiArgument>(getBranch()->getDestBB()->getArgument(argIndex));
}

SILValue getSource() const {
Expand All @@ -591,12 +593,10 @@ struct PhiValue {
PhiValue() = default;

PhiValue(SILValue value) {
auto *blockArg = dyn_cast<SILPhiArgument>(value);
if (!blockArg || !blockArg->isPhi())
return;

phiBlock = blockArg->getParent();
argIndex = blockArg->getIndex();
if (auto *blockArg = SILArgument::asPhi(value)) {
phiBlock = blockArg->getParent();
argIndex = blockArg->getIndex();
}
}

explicit operator bool() const { return phiBlock != nullptr; }
Expand All @@ -612,8 +612,12 @@ struct PhiValue {
}

Operand *getOperand(SILBasicBlock *predecessor) {
auto *branch = cast<BranchInst>(predecessor->getTerminator());
return &branch->getAllOperands()[argIndex];
auto *term = predecessor->getTerminator();
if (auto *branch = dyn_cast<BranchInst>(term)) {
return &branch->getAllOperands()[argIndex];
}
// TODO: Support CondBr for legacy reasons
return cast<CondBranchInst>(term)->getOperandForDestBB(phiBlock, argIndex);
}

operator SILValue() const { return getValue(); }
Expand Down
8 changes: 2 additions & 6 deletions include/swift/SIL/SILBuilder.h
Original file line number Diff line number Diff line change
Expand Up @@ -831,12 +831,8 @@ class SILBuilder {
}

EndBorrowInst *createEndBorrow(SILLocation loc, SILValue borrowedValue) {
if (auto *arg = dyn_cast<SILPhiArgument>(borrowedValue)) {
if (auto *ti = arg->getSingleTerminator()) {
assert(!ti->isTransformationTerminator() &&
"Transforming terminators do not have end_borrow");
}
}
assert(!SILArgument::isTerminatorResult(borrowedValue) &&
"terminator results do not have end_borrow");
return insert(new (getModule())
EndBorrowInst(getSILDebugLocation(loc), borrowedValue));
}
Expand Down
52 changes: 43 additions & 9 deletions include/swift/SIL/SILInstruction.h
Original file line number Diff line number Diff line change
Expand Up @@ -1182,6 +1182,9 @@ class OwnershipForwardingMixin {
///
/// The ownership kind is set on construction and afterwards must be changed
/// explicitly using setOwnershipKind().
///
/// TODO: This name is extremely misleading because it may apply to an
/// operation that has no operand at all, like `enum .None`.
class FirstArgOwnershipForwardingSingleValueInst
: public SingleValueInstruction,
public OwnershipForwardingMixin {
Expand Down Expand Up @@ -1668,6 +1671,8 @@ class UnaryInstructionBase : public InstructionBase<Kind, Base> {

Operand &getOperandRef() { return Operands[0]; }

const Operand &getOperandRef() const { return Operands[0]; }

ArrayRef<Operand> getAllOperands() const { return Operands.asArray(); }
MutableArrayRef<Operand> getAllOperands() { return Operands.asArray(); }

Expand Down Expand Up @@ -1808,6 +1813,10 @@ class UnaryInstructionWithTypeDependentOperandsBase
return this->getAllOperands()[0];
}

const Operand &getOperandRef() const {
return this->getAllOperands()[0];
}

ArrayRef<Operand> getTypeDependentOperands() const {
return this->getAllOperands().slice(1);
}
Expand Down Expand Up @@ -6203,6 +6212,8 @@ class EnumInst

Operand &getOperandRef() { return OptionalOperand->asArray()[0]; }

const Operand &getOperandRef() const { return OptionalOperand->asArray()[0]; }

ArrayRef<Operand> getAllOperands() const {
return OptionalOperand ? OptionalOperand->asArray() : ArrayRef<Operand>{};
}
Expand Down Expand Up @@ -8384,31 +8395,50 @@ class TermInst : public NonValueInstruction {

TermKind getTermKind() const { return TermKind(getKind()); }

/// Returns true if this is a transformation terminator.
/// Returns true if this terminator may have a result, represented as a block
/// argument in any of its successor blocks.
///
/// The first operand is the transformed source.
bool isTransformationTerminator() const {
/// Phis (whose operands originate from BranchInst terminators) are not
/// terminator results.
///
/// CondBr might produce block arguments for legacy reasons. This is gradually
/// being deprecated. For now, they are considered phis. In OSSA, these "phis"
/// must be trivial and critical edges cannot be present.
bool mayHaveTerminatorResult() const {
switch (getTermKind()) {
case TermKind::UnwindInst:
case TermKind::UnreachableInst:
case TermKind::ReturnInst:
case TermKind::ThrowInst:
case TermKind::YieldInst:
case TermKind::TryApplyInst:
case TermKind::BranchInst:
case TermKind::CondBranchInst:
case TermKind::SwitchValueInst:
case TermKind::BranchInst:
case TermKind::SwitchEnumAddrInst:
case TermKind::DynamicMethodBranchInst:
case TermKind::CheckedCastAddrBranchInst:
case TermKind::AwaitAsyncContinuationInst:
return false;
case TermKind::SwitchEnumInst:
case TermKind::CheckedCastBranchInst:
case TermKind::SwitchEnumInst:
case TermKind::SwitchValueInst:
case TermKind::TryApplyInst:
case TermKind::AwaitAsyncContinuationInst:
case TermKind::DynamicMethodBranchInst:
return true;
}
llvm_unreachable("Covered switch isn't covered.");
}

/// Returns an Operand reference if this terminator forwards ownership of a
/// single operand to a single result for at least one successor
/// block. Otherwise returns nullptr.
///
/// By convention, terminators can forward ownership of at most one operand to
/// at most one result. The operand value might not be directly forwarded. For
/// example, a switch forwards ownership of the enum type into ownership of
/// the payload.
///
/// Postcondition: each successor has zero or one block arguments which
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: pre condition(?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, the post-condition only holds if the a forwarding operand is returned. I can clarify that in the next PR

/// represents the forwaded result.
const Operand *forwardedOperand() const;
};

// Forwards the first operand to a result in each successor block.
Expand Down Expand Up @@ -8442,6 +8472,10 @@ class OwnershipForwardingTermInst : public TermInst,

SILValue getOperand() const { return getAllOperands()[0].get(); }

Operand &getOperandRef() { return getAllOperands()[0]; }

const Operand &getOperandRef() const { return getAllOperands()[0]; }

/// Create a result for this terminator on the given successor block.
SILPhiArgument *createResult(SILBasicBlock *succ, SILType resultTy);
};
Expand Down
7 changes: 7 additions & 0 deletions include/swift/SIL/SILValue.h
Original file line number Diff line number Diff line change
Expand Up @@ -494,6 +494,13 @@ class ValueBase : public SILNode, public SILAllocated<ValueBase> {
}
SILInstruction *getDefiningInstruction();

/// Return the instruction that defines this value, terminator instruction
/// that produces this result, or null if it is not defined by an instruction.
const SILInstruction *getDefiningInstructionOrTerminator() const {
return const_cast<ValueBase*>(this)->getDefiningInstructionOrTerminator();
}
SILInstruction *getDefiningInstructionOrTerminator();

/// Return the SIL instruction that can be used to describe the first time
/// this value is available.
///
Expand Down
31 changes: 14 additions & 17 deletions lib/SIL/IR/SILArgument.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,16 +77,17 @@ SILParameterInfo SILFunctionArgument::getKnownParameterInfo() const {
// SILBlockArgument
//===----------------------------------------------------------------------===//

// FIXME: SILPhiArgument should only refer to branch arguments. They usually
// need to be distinguished from projections and casts. Actual phi block
// arguments are substitutable with their incoming values. It is also needlessly
// expensive to call this helper instead of simply specifying phis with an
// opcode. It results in repeated CFG traversals and repeated, unnecessary
// switching over terminator opcodes.
// FIXME: SILPhiArgument should only refer to phis (values merged from
// BranchInst operands). Phis are directly substitutable with their incoming
// values modulo control flow. They usually need to be distinguished from
// projections and casts. It is needlessly expensive to call this helper instead
// of simply specifying phis with an opcode. It results in repeated CFG
// traversals and repeated, unnecessary switching over terminator opcodes.
bool SILPhiArgument::isPhi() const {
// No predecessors indicates an unreachable block.
// No predecessors indicates an unreachable block. Treat this like a
// degenerate phi so we don't consider it a terminator result.
if (getParent()->pred_empty())
return false;
return true;
Copy link
Contributor

@meg-gupta meg-gupta Dec 11, 2022

Choose a reason for hiding this comment

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

For the same reason as this, shouldn't we return true in the case there is only a single predecessor block below ? https://github.com/apple/swift/blob/bf84d8dfe0df272b32c9a9c17d9d47c4f844440c/lib/SIL/IR/SILArgument.cpp#L92

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When there is a single predecessor, we check for terminator results using
isa<BranchInst>(termInst) || isa<CondBranchInst>(termInst)


// Multiple predecessors require phis.
auto *predBlock = getParent()->getSinglePredecessorBlock();
Expand Down Expand Up @@ -215,7 +216,6 @@ bool SILPhiArgument::getIncomingPhiValues(
return false;

const auto *parentBlock = getParent();
assert(!parentBlock->pred_empty());

unsigned argIndex = getIndex();
for (auto *predBlock : getParent()->getPredecessorBlocks()) {
Expand Down Expand Up @@ -323,14 +323,6 @@ bool SILPhiArgument::getSingleTerminatorOperands(
return true;
}

SILValue SILPhiArgument::getSingleTerminatorOperand() const {
const auto *parentBlock = getParent();
const auto *predBlock = parentBlock->getSinglePredecessorBlock();
if (!predBlock)
return SILValue();
return getSingleTerminatorOperandForPred(parentBlock, predBlock, getIndex());
}

TermInst *SILPhiArgument::getSingleTerminator() const {
auto *parentBlock = getParent();
auto *predBlock = parentBlock->getSinglePredecessorBlock();
Expand All @@ -349,6 +341,11 @@ TermInst *SILPhiArgument::getTerminatorForResult() const {
return nullptr;
}

const Operand *SILArgument::forwardedTerminatorResultOperand() const {
assert(isTerminatorResult() && "API is invalid for phis");
return getSingleTerminator()->forwardedOperand();
}

SILPhiArgument *BranchInst::getArgForOperand(const Operand *oper) {
assert(oper->getUser() == this);
return cast<SILPhiArgument>(
Expand Down
34 changes: 34 additions & 0 deletions lib/SIL/IR/SILInstructions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1617,6 +1617,40 @@ TermInst::getSuccessorBlockArgumentLists() const {
return SuccessorBlockArgumentListTy(getSuccessors(), op);
}

const Operand *TermInst::forwardedOperand() const {
switch (getTermKind()) {
case TermKind::UnwindInst:
case TermKind::UnreachableInst:
case TermKind::ReturnInst:
case TermKind::ThrowInst:
case TermKind::YieldInst:
case TermKind::TryApplyInst:
case TermKind::CondBranchInst:
case TermKind::BranchInst:
case TermKind::SwitchEnumAddrInst:
case TermKind::SwitchValueInst:
case TermKind::DynamicMethodBranchInst:
case TermKind::CheckedCastAddrBranchInst:
case TermKind::AwaitAsyncContinuationInst:
return nullptr;
case TermKind::SwitchEnumInst: {
auto *switchEnum = cast<SwitchEnumInst>(this);
if (!switchEnum->preservesOwnership())
return nullptr;

return &switchEnum->getOperandRef();
}
case TermKind::CheckedCastBranchInst: {
auto *checkedCast = cast<CheckedCastBranchInst>(this);
if (!checkedCast->preservesOwnership())
return nullptr;

return &checkedCast->getOperandRef();
}
}
llvm_unreachable("Covered switch isn't covered.");
}

YieldInst *YieldInst::create(SILDebugLocation loc,
ArrayRef<SILValue> yieldedValues,
SILBasicBlock *normalBB, SILBasicBlock *unwindBB,
Expand Down
10 changes: 10 additions & 0 deletions lib/SIL/IR/SILValue.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,16 @@ SILInstruction *ValueBase::getDefiningInstruction() {
return nullptr;
}

SILInstruction *ValueBase::getDefiningInstructionOrTerminator() {
if (auto *inst = dyn_cast<SingleValueInstruction>(this))
return inst;
if (auto *result = dyn_cast<MultipleValueInstructionResult>(this))
return result->getParent();
if (auto *result = SILArgument::isTerminatorResult(this))
return result->getSingleTerminator();
return nullptr;
}

SILInstruction *ValueBase::getDefiningInsertionPoint() {
if (auto *inst = getDefiningInstruction())
return inst;
Expand Down
Loading