-
Notifications
You must be signed in to change notification settings - Fork 10.6k
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
Conversation
71b9a48 to
0709689
Compare
|
@swift-ci test |
include/swift/SIL/SILBasicBlock.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see PhiOperand constructor still works with branch operands. Any reason this had to change ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, should the PhiOperand constructor be updated to handle cond_br in non-ossa?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
lib/SIL/IR/SILArgument.cpp
Outdated
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
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.
0709689 to
f9861ec
Compare
|
@swift-ci smoke test |
meg-gupta
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. LGTM
| /// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: pre condition(?)
There was a problem hiding this comment.
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
Add SILValue::getDefiningInstructionOrTerminator().
Add APIs for terminator results that forward ownership.