[ownership] Add a new class BorrowScopeIntroducer that enables code t… #27129
Conversation
|
@swift-ci smoke test |
gottesmm
force-pushed
the
pr-8fee81e00841b8044b3368fabdcd84dd5fbd5f68
branch
from
d51b6a6 to
eb7b5da
Compare
|
@swift-ci smoke test |
|
@swift-ci smoke test and merge |
lib/SIL/OwnershipUtils.cpp
Outdated
| llvm_unreachable("Covered switch isn't covered?!"); | ||
| } | ||
|
|
||
| bool BorrowScopeIntroducer::visitScopeEndingInstructions( |
There was a problem hiding this comment.
I've been making a lot of use of visitor patterns that return false as soon as a visitor returns false. In general, this allows the visitor to bail out, early exit, and return that information to the client. In that pattern, when no visitors are called, true must be returned to indicate that everything is handled. I think returning false here to mean something specific in this context does not respect the more general expectation. It would be better to either add hasLocalScope method or return a more descriptive enum result to indicate that function arguments are special here.
There was a problem hiding this comment.
I am fine with returning a more descriptive enum.
There was a problem hiding this comment.
@atrick was thinking about this. The key thing to note here is that by construction we know that we will always be able to visit everything. So perhaps the way to make this clearer is as you said do the hasLocalScope, put in an assert that we only call this if we have a local scope.
lib/SIL/OwnershipUtils.cpp
Outdated
| return os; | ||
| } | ||
|
|
||
| bool BorrowScopeIntroducer::areInstructionsWithinScope( |
There was a problem hiding this comment.
I'll be happy when BranchPropagatedUser can go away, and valueHasLinearLifetime is a object that holds its own state. Until then, this is fine.
There was a problem hiding this comment.
yes. I am with you. I just wanted to abstract this a way since it is a general operation we have been performing. I can abstract it out.
There was a problem hiding this comment.
@atrick for some cases I don't think it will go away, but in a subsequent commit, I have a small helper that in the case where we /know/ that we are never going to pass a cond_br we can just convert the ArrayRef<SILInstruction *> to ArrayRef. The reason why this is safe is BranchPropagatedUser is just a pointer int pair internally and if it is not storing a cond_br, then it is exactly bitwise the same as the underlying SILInstruction *. I can make the helper safe to use by performing a dynamic check if we have SILInstruction and a compile time check if we have an ArrayRef.
There was a problem hiding this comment.
That fact that cond_br still has arguments is dumb but it's unrelated to this PR. I'm just anxious for that to go away so code like this can be vastly simpler.
There was a problem hiding this comment.
Just responding to your comments here.
FYI: In a forthcoming commit, I am going to rename this class to BorrowScopeIntroducingValue. The reason why is that we also need to consider operands as borrow scope introducer concepts since we are allowing for the use of an instruction to introduce a borrow scope. Consider begin_apply. So I need to think about how to layer these with an isa relationship.
lib/SIL/OwnershipUtils.cpp
Outdated
| return os; | ||
| } | ||
|
|
||
| bool BorrowScopeIntroducer::areInstructionsWithinScope( |
There was a problem hiding this comment.
yes. I am with you. I just wanted to abstract this a way since it is a general operation we have been performing. I can abstract it out.
lib/SIL/OwnershipUtils.cpp
Outdated
| llvm_unreachable("Covered switch isn't covered?!"); | ||
| } | ||
|
|
||
| bool BorrowScopeIntroducer::visitScopeEndingInstructions( |
There was a problem hiding this comment.
I am fine with returning a more descriptive enum.
…code to work abstractly with values that introduce a new borrow scope. The idea is that this can be used to work with things like load_borrow, begin_borrow, SILFunctionArgument, results of begin_apply(in the future) and the like in a generic way using exhaustive switches to make sure this code stays up to date. I refactored code in SemanticARCOpts and some utilities in OwnershipUtils.cpp to use these new APIs. The code looks a lot nicer and should be quite easy to expand to handle new borrow introducers (e.x.: end_apply).
gottesmm
force-pushed
the
pr-8fee81e00841b8044b3368fabdcd84dd5fbd5f68
branch
from
eb7b5da to
bb4df03
Compare
|
@swift-ci smoke test and merge |
1 similar comment
|
@swift-ci smoke test and merge |
…o work abstractly with values that introduce a new borrow scope.
The idea is that this can be used to work with things like load_borrow,
begin_borrow, SILFunctionArgument, BeginApply (in the future) and the like in a
generic way using exhaustive switches to make sure this code stays up to date.
I refactored code in SemanticARCOpts and some utilities in OwnershipUtils.cpp to
use these new APIs. The code looks a lot nicer and should be quite easy to
expand to handle new borrow introducers (e.x.: end_apply).
Should be a pure refactor so: NFCI.