APIs for terminator results that forward ownership #62493
Conversation
atrick
force-pushed
the
term-result-operand
branch
2 times, most recently
from
71b9a48 to
0709689
Compare
|
@swift-ci test |
include/swift/SIL/SILBasicBlock.h
Outdated
| return cast<BranchInst>(predBlock->getTerminator()); | ||
| // TODO: This should eventually be limited to BranchInst. | ||
| TermInst *getBranch() const { | ||
| return predBlock->getTerminator(); |
There was a problem hiding this comment.
I see PhiOperand constructor still works with branch operands. Any reason this had to change ?
There was a problem hiding this comment.
The problem with these abstractions is that we still don't have all the necessary SIL restrictions. In particular, OSSA still supports CondBr arguments for trivial types. We deferred doing this work because OSSA utilities generally ignores trivial values anyway. Until we fix this, \p PhiOperand might be invalid for trivial phis.
I started to add support for CondBr arguments everywhere, but reconsidered. I just changed the getBranch method back to returning BranchInst as before.
include/swift/SIL/SILBasicBlock.h
Outdated
| auto *branch = cast<BranchInst>(predBlock->getTerminator()); | ||
| return cast<SILPhiArgument>(branch->getDestBB()->getArgument(argIndex)); | ||
| if (auto *branch = dyn_cast<BranchInst>(predBlock->getTerminator())) | ||
| return cast<SILPhiArgument>(branch->getDestBB()->getArgument(argIndex)); |
There was a problem hiding this comment.
Same here, should the PhiOperand constructor be updated to handle cond_br in non-ossa?
There was a problem hiding this comment.
For now, I fixed PhiOperand to be invalid for CondBr. We'll either need need prohibit CondBr arguments or change the representation of this abstraction. Prohibiting CondBr arguments would be far simpler in the long term.
| if (getParent()->pred_empty()) | ||
| return false; | ||
| return true; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
When there is a single predecessor, we check for terminator results using
isa<BranchInst>(termInst) || isa<CondBranchInst>(termInst)
This allows code to handle terminator results similar to any other instruction result. Data flow should generally handle terminator results like any other instruction that may forward operand ownership to its results. The fact that it is represented as a block argument is an implementation detail that gets in the way of conceptual simplicity.
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.
atrick
force-pushed
the
term-result-operand
branch
from
0709689 to
f9861ec
Compare
|
@swift-ci smoke test |
| /// example, a switch forwards ownership of the enum type into ownership of | ||
| /// the payload. | ||
| /// | ||
| /// Postcondition: each successor has zero or one block arguments which |
There was a problem hiding this comment.
nit: pre condition(?)
There was a problem hiding this comment.
Well, the post-condition only holds if the a forwarding operand is returned. I can clarify that in the next PR
Add SILValue::getDefiningInstructionOrTerminator().
Add APIs for terminator results that forward ownership.