[ownership] Only allow BranchPropagatedUser to be constructed from Operands. #27316
Conversation
I need this for some refactorings I am doing around BranchPropagatedUser. We already had ApplySite::getCallee() which just returned the value of the operand. I refactored it to call this instead.
I am doing some refactoring on BranchPropagatedUser that is requiring me to work a lot more with arguments and I found a need for this to avoid trafficing in operand indices. As part of this I also needed to write the helper getConditionOperand() to get the underlying operand. I refactored the already preset getCondition() to use this helper instead since it was just semantically that method + get operand's value.
…erands. This commit only changes how BranchPropagatedUser is constructed and does not change the internal representation. This is a result of my noticing that BranchPropagatedUser could also use an operand internally to represent its state. To simplify how I am making the change, I am splitting the change into two PRs that should be easy to validate: 1. A commit that maps how the various users of BranchPropagatedUser have been constructing BPUs to a single routine that takes an Operand. This leaves BranchPropagatedUser's internal state alone as well as its user the Linear Lifetime Checker. 2. A second commit that changes the internal bits of the BranchPropagatedUser to store an Operand instead of a PointerUnion. This will allow me to use the first commit to validate the second.
|
@swift-ci test |
|
Once this is done, I am going to be able to introduce a new BorrowScopeIntroducingOperand higher level construct. This is going to let me change the place I marked with HERE, to use that to add support for checking an owned value being used in a guaranteed position of begin_apply as well as add support for adding in an implicit scope for mark_dependence and a new instruction called end_mark_dependence that I am going to add once this lands. |
| nonLifetimeEndingUsers[i]->getUser())) { | ||
| for (auto *use : bbi->getUses()) { | ||
| if (isa<EndBorrowInst>(use->getUser())) { | ||
| implicitRegularUsers.push_back(use); |
|
It should also let me add support pretty easily for adding ref_element_addr users to the verification here. |
| @@ -96,6 +107,19 @@ class BranchPropagatedUser { | |||
| NumLowBitsAvailable = | |||
| llvm::PointerLikeTypeTraits<InnerTy>::NumLowBitsAvailable | |||
| }; | |||
|
|
|||
| private: | |||
| BranchPropagatedUser(SILInstruction *inst) : user(inst) { | |||
There was a problem hiding this comment.
These are still used by some internal details of BranchPropagatedUser. I wanted to keep this commit really clean and easy to read, so I made these private rather than modifying the in-class callers.
|
Also note, my hope is that this is a stepping stone to getting rid of BranchPropagatedUser (or at least hiding it). |
|
Build failed |
|
@swift-ci test linux platform |
|
Seems fine. I don't think you need to put effort into getting rid of BranchPropagatedUser. The underlying problem goes away when cond_br args go away. I had a patch for that about a year ago but didn't get around to updating all the SIL tests. I really think I'll be able to revive that soon. |
|
@atrick I am doing this for a bigger reason. I need this to add a BorrowScopeIntroducingOperand higher level construct. |
This commit only changes how BranchPropagatedUser is constructed and does not
change the internal representation. This is a result of my noticing that
BranchPropagatedUser could also use an operand internally to represent its
state. To simplify how I am making the change, I am splitting the change into
two PRs that should be easy to validate:
A commit that maps how the various users of BranchPropagatedUser have been
constructing BPUs to a single routine that takes an Operand. This leaves
BranchPropagatedUser's internal state alone as well as its user the Linear
Lifetime Checker.
A second commit that changes the internal bits of the BranchPropagatedUser to
store an Operand instead of a PointerUnion.
This will allow me to use the first commit to validate the second.
NOTE: I split off two small helper adding commits.