Skip to content

Commit 95de4d2

Browse files
committed
[pmo] Fix load [copy] like I fixed load_borrow.
This involved fixing a few issues exposed by trying to use the load_borrow code with load [copy] that were caught in our tests by the ownership verifier. Specifically: 1. In getUnderlyingBorrowIntroducingValues we were first checking if a user was a guaranteed forwarding instruction and then doing a check if our value was a value with None ownership that we want to skip. This caused weird behavior where one could get the wrong borrow introducers if that trivial value came from a different guaranteed value. 2. I found via a test case in predictable_memaccess_opts that we needed to insert compensating destroys for phi nodes after we process all of the incoming values. The reason why this is needed is that multiple phi nodes could use the same incoming value. In such a case, we need to treat all of the copy_values inserted for each phi node as users of that incoming value to prevent us from inserting too many destroy_value. Consider: ``` bb0: %0 = copy_value cond_br ..., bb1, bb2 bb1: br bb3(%0) bb2: br bb4(%0) ``` If we processed the phi args in bb3, bb4 separately, we would insert an extra 2 destroys in bb1, bb2. The implementation works by processing each phi and for each incoming value of the phi appending the value, copy we made for the value to an array. We then stable sort the array only by value. This then allows us to process ranges of copies with the same underlying incoming value in a 2nd loop taking advantage of all of the copies for an incoming value being contiguous in said array. We still lifetime extend to the load/insert destroy_value for the actual phi (not its incoming values) in that first loop. 3. I tightened up the invariant in the AvailableValueAggregator that it always returns values that are newly copied unless we are performing a take. This involved changing the code in handlePrimitiveValues to always insert copies when ownership is enabled. 4. I tightened up the identification of intermediate phi nodes by instead of just checking if the phi node has a single terminator user to instead it having a single terminator user that has a successor with an inserted phi node that we know about.
1 parent b8a1ecd commit 95de4d2

File tree

4 files changed

+867
-188
lines changed

4 files changed

+867
-188
lines changed

lib/SIL/OwnershipUtils.cpp

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,13 @@ bool swift::getUnderlyingBorrowIntroducingValues(
163163
continue;
164164
}
165165

166+
// If v produces .none ownership, then we can ignore it. It is important
167+
// that we put this before checking for guaranteed forwarding instructions,
168+
// since we want to ignore guaranteed forwarding instructions that in this
169+
// specific case produce a .none value.
170+
if (v.getOwnershipKind() == ValueOwnershipKind::None)
171+
continue;
172+
166173
// Otherwise if v is an ownership forwarding value, add its defining
167174
// instruction
168175
if (isGuaranteedForwardingValue(v)) {
@@ -173,10 +180,9 @@ bool swift::getUnderlyingBorrowIntroducingValues(
173180
continue;
174181
}
175182

176-
// If v produces any ownership, then we can ignore it. Otherwise, we need to
177-
// return false since this is an introducer we do not understand.
178-
if (v.getOwnershipKind() != ValueOwnershipKind::None)
179-
return false;
183+
// Otherwise, this is an introducer we do not understand. Bail and return
184+
// false.
185+
return false;
180186
}
181187

182188
return true;

0 commit comments

Comments
 (0)