Specifically,
copyAndExtendForLifetimeEndingRAUW -> createPlusOneCopy(value, oldConsumingUse)
copyAndExtendForNonLifetimeEndingRAUW -> createPlusZeroCopy(value, nonLifetimeEndingUses)
copyBorrowAndExtendForRAUW -> createPlusZeroBorrow(value, nonLifetimeEndingUses)
Beyond being shorter, the PlusOne vs PlusZero mnemonic is telling the caller
whether or not the caller is responsible for cleaning up the returned
copy_value. In the PlusZero cases, we assume that all uses are non-lifetime
ending and we need to insert all destroy_value at the liveness points of the
value.
In the PlusOne case in contrast, we are supposed to make a copy of value and the
complete oldConsumingUse's block into a joint post-dominance set of blocks. Then
we insert destroy_values for our new copy into the completion blocks that we
found and leave cleaning up value along paths through oldConsumingUse to our
user. In this sense the value is a PlusOne value that our user must clean up for
us (sorta makes me think about ManagedValue).
The only operational change here is that I needed to be able to grab the module
from the SILValue so I could see if we were in Raw SIL or not. I realized the
only case where we could not get the module is from SILUndef and at this point
in the code we know we are going to bail already for SILUndef. This is because
we already know our new value doesn't have OwnershipKind::None and we don't
replace OwnershipKind::None things with non-OwnershipKind::None things since I
haven't implemented support for that corner case yet (but will with time).
Once I realized the previous paragraph, I was able to add support without issue.
This allows for me to do a couple of things improving quality/correctness/ease of use:
1. I reimplemented InstMod's RAUW and RAUW/erase helpers on top of
setUseValue/deleteInst. Beyond allowing the caller to specify less things, we
gain an orthogonality preventing bugs like overriding erase/RAUW but not
overriding erase or having the erase used in erase/RAUW act differently than
the erase for deleteInst.
2. There were a bunch of places using InstModCallback that also were setting
uses without having the ability for InstModCallbacks perform it (since it
only supported RAUW). This is an anti-pattern and could cause subtle bugs to
be introduced by appropriate state in the caller not being updated.
Specifically before this PR, if a caller did not customize a specific callback
of InstModCallbacks, we would store a static default std::function into
InstModCallbacks. This means that we always would have an indirect jump. That is
unfortunate since this code is often called in loops.
In this PR, I eliminate this problem by:
1. I made all of the actual callback std::function in InstModCallback private
and gave them a "Func" postfix (e.x.: deleteInst -> deleteInstFunc).
2. I created public methods with the old callback names to actually call the
callbacks. This ensured that as long as we are not escaping callbacks from
InstModCallback, this PR would not result in the need for any source changes
since we are changing a call of a std::function field to a call to a method.
3. I changed all of the places that were escaping inst mod's callbacks to take
an InstModCallback. We shouldn't be doing that anyway.
4. I changed the default value of each callback in InstModCallbacks to be a
nullptr and changed the public helper methods to check if a callback is
null. If the callback is not null, it is called, otherwise the getter falls
back to an inline default implementation of the operation.
All together this means that the cost of a plain InstModCallback is reduced and
one pays an indirect function cost price as one customizes it further which is
better scalability.
P.S. as a little extra thing, I added a madeChange field onto the
InstModCallback. Now that we have the helpers calling the callbacks, I can
easily insert instrumentation like this, allowing for users to pass in
InstModCallback and see if anything was RAUWed without needing to specify a
callback.
When we are populating transitive users while handling guaranteed values
in ownership rauw, we were including values with none ownership.
Example : rauw of %2 with a dominating value
Here %arg1 was also considered a transitive use
%2 = struct_extract %0 : $StructWithEnum2, #StructWithEnum2.val
%copy = copy_value %2 : $FakeOptional2
switch_enum %2 : $FakeOptional2, case #FakeOptional2.some1!enumelt:bb5, case #FakeOptional2.some2!enumelt:bb6
bb5(%arg1 : $UInt):
br bb7(%arg1 : $UInt)
bb6(%arg2 : @guaranteed $Klass):
%4 = unchecked_trivial_bit_cast %arg2 : $Klass to $UInt
br bb7(%4 : $UInt)
This is incorrect because %arg1 is a trivial value, and this also
leads to ValueLifetimeAnalysis needing a split for finding a frontier
for the use of %arg1 in the branch instruction. In ossa, we should never
have to split edges for finding frontiers, because we do not have critical
edges.
Branch instructions and frontiers can have
LocationKind::ReturnKind/ImplicitReturnKind which are not correct
locations to use for copy_value/destroy_value etc
This is a generic API that when ownership is enabled allows one to replace all
uses of a value with a value with a differing ownership by transforming/lifetime
extending as appropriate.
This API supports all pairings of ownership /except/ replacing a value with
OwnershipKind::None with a value without OwnershipKind::None. This is a more
complex optimization that we do not support today. As a result, we include on
our state struct a helper routine that callers can use to know if the two values
that they want to process can be handled by the algorithm.
My moticiation is to use this to to update InstSimplify and SILCombiner in a
less bug prone way rather than just turn stuff off.
Noting that this transformation inserts ownership instructions, I have made sure
to test this API in two ways:
1. With Mandatory Combiner alone (to make sure it works period).
2. With Mandatory Combiner + Semantic ARC Opts to make sure that we can
eliminate the extra ownership instructions it inserts.
As one can see from the tests, the optimizer today is able to handle all of
these transforms except one conditional case where I need to eliminate a dead
phi arg. I have a separate branch that hits that today but I have exposed unsafe
behavior in ClosureLifetimeFixup that I need to fix first before I can land
that. I don't want that to stop this PR since I think the current low level ARC
optimizer may be able to help me here since this is a simple transform it does
all of the time.
