Skip to content

Commit f9861ec

Browse files
atrickatrick
committed
Add APIs for terminator results that forward ownership.
Add TermInst::forwardedOperand. Add SILArgument::forwardedTerminatorResultOperand. This API will be moved into a proper TerminatorResult abstraction. Remove getSingleTerminatorOperand, which could be misused because it's not necessarilly forwarding ownership. Remove the isTransformationTerminator API, which is not useful or well defined. Rewrite several instances of complex logic to handle block arguments with the simple terminator result API. This defines away potential bugs where we don't detect casts that perform implicit conversion. Replace uses of the SILPhiArgument type and code that explicitly handle block arguments. Control flow is irrelevant in these situations. SILPhiArgument needs to be deleted ASAP. Instead, use simple APIs like SILArgument::isTerminatorResult(). Eventually this will be replaced by a TerminatorResult type.
1 parent fe44dce commit f9861ec

File tree

14 files changed

+181
-207
lines changed

14 files changed

+181
-207
lines changed

‎include/swift/SIL/SILArgument.h

Lines changed: 14 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ class SILArgument : public ValueBase {
126126
unsigned getIndex() const;
127127

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

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

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

190-
/// If this SILArgument's parent block has a single predecessor whose
191-
/// terminator has a single operand, return the incoming operand of the
192-
/// predecessor's terminator. Returns SILValue() otherwise. Note that for
193-
/// some predecessor terminators the incoming value is not exactly the
194-
/// argument value. E.g. the incoming value for a switch_enum payload argument
195-
/// is the enum itself (the operand of the switch_enum).
196-
SILValue getSingleTerminatorOperand() const;
197-
198195
/// If this SILArgument's parent block has a single predecessor whose
199196
/// terminator has a single operand, return that terminator.
200197
TermInst *getSingleTerminator() const;
@@ -203,6 +200,13 @@ class SILArgument : public ValueBase {
203200
/// otherwise return nullptr.
204201
TermInst *getTerminatorForResult() const;
205202

203+
/// If this terminator result forwards an operand, then return it.
204+
///
205+
/// Precondition: this->isTerminatorResult()
206+
///
207+
/// TODO: Move this and other APIs into a TerminatorResult abstraction.
208+
const Operand *forwardedTerminatorResultOperand() const;
209+
206210
/// Return the SILArgumentKind of this argument.
207211
SILArgumentKind getKind() const {
208212
return SILArgumentKind(ValueBase::getKind());
@@ -310,14 +314,6 @@ class SILPhiArgument : public SILArgument {
310314
SmallVectorImpl<std::pair<SILBasicBlock *, SILValue>>
311315
&returnedSingleTermOperands) const;
312316

313-
/// If this SILArgument's parent block has a single predecessor whose
314-
/// terminator has a single operand, return the incoming operand of the
315-
/// predecessor's terminator. Returns SILValue() otherwise. Note that for
316-
/// some predecessor terminators the incoming value is not exactly the
317-
/// argument value. E.g. the incoming value for a switch_enum payload argument
318-
/// is the enum itself (the operand of the switch_enum).
319-
SILValue getSingleTerminatorOperand() const;
320-
321317
/// If this SILArgument's parent block has a single predecessor whose
322318
/// terminator has a single operand, return that terminator.
323319
TermInst *getSingleTerminator() const;
@@ -406,7 +402,7 @@ class SILFunctionArgument : public SILArgument {
406402
//===----------------------------------------------------------------------===//
407403

408404
/// Return non-null if \p value is a real phi argument.
409-
inline SILPhiArgument *SILArgument::isPhi(SILValue value) {
405+
inline SILPhiArgument *SILArgument::asPhi(SILValue value) {
410406
if (auto *arg = dyn_cast<SILPhiArgument>(value)) {
411407
if (arg->isPhi())
412408
return arg;

‎include/swift/SIL/SILBasicBlock.h

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -440,7 +440,7 @@ public SwiftObjectHeader {
440440
/// Whether the block has any phi arguments.
441441
///
442442
/// Note that a block could have an argument and still return false. The
443-
/// argument must also satisfy SILPhiArgument::isPhiArgument.
443+
/// argument must also satisfy SILPhiArgument::isPhi.
444444
bool hasPhi() const;
445445

446446
//===--------------------------------------------------------------------===//
@@ -541,6 +541,8 @@ struct PhiOperand {
541541

542542
PhiOperand() = default;
543543

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

569571
SILPhiArgument *getValue() const {
570-
auto *branch = cast<BranchInst>(predBlock->getTerminator());
571-
return cast<SILPhiArgument>(branch->getDestBB()->getArgument(argIndex));
572+
return
573+
cast<SILPhiArgument>(getBranch()->getDestBB()->getArgument(argIndex));
572574
}
573575

574576
SILValue getSource() const {
@@ -591,12 +593,10 @@ struct PhiValue {
591593
PhiValue() = default;
592594

593595
PhiValue(SILValue value) {
594-
auto *blockArg = dyn_cast<SILPhiArgument>(value);
595-
if (!blockArg || !blockArg->isPhi())
596-
return;
597-
598-
phiBlock = blockArg->getParent();
599-
argIndex = blockArg->getIndex();
596+
if (auto *blockArg = SILArgument::asPhi(value)) {
597+
phiBlock = blockArg->getParent();
598+
argIndex = blockArg->getIndex();
599+
}
600600
}
601601

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

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

619623
operator SILValue() const { return getValue(); }

‎include/swift/SIL/SILBuilder.h

Lines changed: 2 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -831,12 +831,8 @@ class SILBuilder {
831831
}
832832

833833
EndBorrowInst *createEndBorrow(SILLocation loc, SILValue borrowedValue) {
834-
if (auto *arg = dyn_cast<SILPhiArgument>(borrowedValue)) {
835-
if (auto *ti = arg->getSingleTerminator()) {
836-
assert(!ti->isTransformationTerminator() &&
837-
"Transforming terminators do not have end_borrow");
838-
}
839-
}
834+
assert(!SILArgument::isTerminatorResult(borrowedValue) &&
835+
"terminator results do not have end_borrow");
840836
return insert(new (getModule())
841837
EndBorrowInst(getSILDebugLocation(loc), borrowedValue));
842838
}

‎include/swift/SIL/SILInstruction.h

Lines changed: 43 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1182,6 +1182,9 @@ class OwnershipForwardingMixin {
11821182
///
11831183
/// The ownership kind is set on construction and afterwards must be changed
11841184
/// explicitly using setOwnershipKind().
1185+
///
1186+
/// TODO: This name is extremely misleading because it may apply to an
1187+
/// operation that has no operand at all, like `enum .None`.
11851188
class FirstArgOwnershipForwardingSingleValueInst
11861189
: public SingleValueInstruction,
11871190
public OwnershipForwardingMixin {
@@ -1668,6 +1671,8 @@ class UnaryInstructionBase : public InstructionBase<Kind, Base> {
16681671

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

1674+
const Operand &getOperandRef() const { return Operands[0]; }
1675+
16711676
ArrayRef<Operand> getAllOperands() const { return Operands.asArray(); }
16721677
MutableArrayRef<Operand> getAllOperands() { return Operands.asArray(); }
16731678

@@ -1808,6 +1813,10 @@ class UnaryInstructionWithTypeDependentOperandsBase
18081813
return this->getAllOperands()[0];
18091814
}
18101815

1816+
const Operand &getOperandRef() const {
1817+
return this->getAllOperands()[0];
1818+
}
1819+
18111820
ArrayRef<Operand> getTypeDependentOperands() const {
18121821
return this->getAllOperands().slice(1);
18131822
}
@@ -6203,6 +6212,8 @@ class EnumInst
62036212

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

6215+
const Operand &getOperandRef() const { return OptionalOperand->asArray()[0]; }
6216+
62066217
ArrayRef<Operand> getAllOperands() const {
62076218
return OptionalOperand ? OptionalOperand->asArray() : ArrayRef<Operand>{};
62086219
}
@@ -8384,31 +8395,50 @@ class TermInst : public NonValueInstruction {
83848395

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

8387-
/// Returns true if this is a transformation terminator.
8398+
/// Returns true if this terminator may have a result, represented as a block
8399+
/// argument in any of its successor blocks.
83888400
///
8389-
/// The first operand is the transformed source.
8390-
bool isTransformationTerminator() const {
8401+
/// Phis (whose operands originate from BranchInst terminators) are not
8402+
/// terminator results.
8403+
///
8404+
/// CondBr might produce block arguments for legacy reasons. This is gradually
8405+
/// being deprecated. For now, they are considered phis. In OSSA, these "phis"
8406+
/// must be trivial and critical edges cannot be present.
8407+
bool mayHaveTerminatorResult() const {
83918408
switch (getTermKind()) {
83928409
case TermKind::UnwindInst:
83938410
case TermKind::UnreachableInst:
83948411
case TermKind::ReturnInst:
83958412
case TermKind::ThrowInst:
83968413
case TermKind::YieldInst:
8397-
case TermKind::TryApplyInst:
8398-
case TermKind::BranchInst:
83998414
case TermKind::CondBranchInst:
8400-
case TermKind::SwitchValueInst:
8415+
case TermKind::BranchInst:
84018416
case TermKind::SwitchEnumAddrInst:
8402-
case TermKind::DynamicMethodBranchInst:
84038417
case TermKind::CheckedCastAddrBranchInst:
8404-
case TermKind::AwaitAsyncContinuationInst:
84058418
return false;
8406-
case TermKind::SwitchEnumInst:
84078419
case TermKind::CheckedCastBranchInst:
8420+
case TermKind::SwitchEnumInst:
8421+
case TermKind::SwitchValueInst:
8422+
case TermKind::TryApplyInst:
8423+
case TermKind::AwaitAsyncContinuationInst:
8424+
case TermKind::DynamicMethodBranchInst:
84088425
return true;
84098426
}
84108427
llvm_unreachable("Covered switch isn't covered.");
84118428
}
8429+
8430+
/// Returns an Operand reference if this terminator forwards ownership of a
8431+
/// single operand to a single result for at least one successor
8432+
/// block. Otherwise returns nullptr.
8433+
///
8434+
/// By convention, terminators can forward ownership of at most one operand to
8435+
/// at most one result. The operand value might not be directly forwarded. For
8436+
/// example, a switch forwards ownership of the enum type into ownership of
8437+
/// the payload.
8438+
///
8439+
/// Postcondition: each successor has zero or one block arguments which
8440+
/// represents the forwaded result.
8441+
const Operand *forwardedOperand() const;
84128442
};
84138443

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

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

8475+
Operand &getOperandRef() { return getAllOperands()[0]; }
8476+
8477+
const Operand &getOperandRef() const { return getAllOperands()[0]; }
8478+
84458479
/// Create a result for this terminator on the given successor block.
84468480
SILPhiArgument *createResult(SILBasicBlock *succ, SILType resultTy);
84478481
};

‎lib/SIL/IR/SILArgument.cpp

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -77,16 +77,17 @@ SILParameterInfo SILFunctionArgument::getKnownParameterInfo() const {
7777
// SILBlockArgument
7878
//===----------------------------------------------------------------------===//
7979

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

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

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

220220
unsigned argIndex = getIndex();
221221
for (auto *predBlock : getParent()->getPredecessorBlocks()) {
@@ -323,14 +323,6 @@ bool SILPhiArgument::getSingleTerminatorOperands(
323323
return true;
324324
}
325325

326-
SILValue SILPhiArgument::getSingleTerminatorOperand() const {
327-
const auto *parentBlock = getParent();
328-
const auto *predBlock = parentBlock->getSinglePredecessorBlock();
329-
if (!predBlock)
330-
return SILValue();
331-
return getSingleTerminatorOperandForPred(parentBlock, predBlock, getIndex());
332-
}
333-
334326
TermInst *SILPhiArgument::getSingleTerminator() const {
335327
auto *parentBlock = getParent();
336328
auto *predBlock = parentBlock->getSinglePredecessorBlock();
@@ -349,6 +341,11 @@ TermInst *SILPhiArgument::getTerminatorForResult() const {
349341
return nullptr;
350342
}
351343

344+
const Operand *SILArgument::forwardedTerminatorResultOperand() const {
345+
assert(isTerminatorResult() && "API is invalid for phis");
346+
return getSingleTerminator()->forwardedOperand();
347+
}
348+
352349
SILPhiArgument *BranchInst::getArgForOperand(const Operand *oper) {
353350
assert(oper->getUser() == this);
354351
return cast<SILPhiArgument>(

‎lib/SIL/IR/SILInstructions.cpp

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1617,6 +1617,40 @@ TermInst::getSuccessorBlockArgumentLists() const {
16171617
return SuccessorBlockArgumentListTy(getSuccessors(), op);
16181618
}
16191619

1620+
const Operand *TermInst::forwardedOperand() const {
1621+
switch (getTermKind()) {
1622+
case TermKind::UnwindInst:
1623+
case TermKind::UnreachableInst:
1624+
case TermKind::ReturnInst:
1625+
case TermKind::ThrowInst:
1626+
case TermKind::YieldInst:
1627+
case TermKind::TryApplyInst:
1628+
case TermKind::CondBranchInst:
1629+
case TermKind::BranchInst:
1630+
case TermKind::SwitchEnumAddrInst:
1631+
case TermKind::SwitchValueInst:
1632+
case TermKind::DynamicMethodBranchInst:
1633+
case TermKind::CheckedCastAddrBranchInst:
1634+
case TermKind::AwaitAsyncContinuationInst:
1635+
return nullptr;
1636+
case TermKind::SwitchEnumInst: {
1637+
auto *switchEnum = cast<SwitchEnumInst>(this);
1638+
if (!switchEnum->preservesOwnership())
1639+
return nullptr;
1640+
1641+
return &switchEnum->getOperandRef();
1642+
}
1643+
case TermKind::CheckedCastBranchInst: {
1644+
auto *checkedCast = cast<CheckedCastBranchInst>(this);
1645+
if (!checkedCast->preservesOwnership())
1646+
return nullptr;
1647+
1648+
return &checkedCast->getOperandRef();
1649+
}
1650+
}
1651+
llvm_unreachable("Covered switch isn't covered.");
1652+
}
1653+
16201654
YieldInst *YieldInst::create(SILDebugLocation loc,
16211655
ArrayRef<SILValue> yieldedValues,
16221656
SILBasicBlock *normalBB, SILBasicBlock *unwindBB,

0 commit comments

Comments
 (0)