[ownership] Do not RAUW destructures if replacement value is a .none non-trivial type. #27902
Conversation
|
@swift-ci test |
| isa<MultipleValueInstructionResult>(r) && | ||
| r.getOwnershipKind() == ValueOwnershipKind::Owned && | ||
| C.getOwnershipKind() == ValueOwnershipKind::None; | ||
| r->replaceAllUsesWith(C); |
There was a problem hiding this comment.
I find it hard to reason about correctness of this RAUW w.r.t. ownership. If it were expressed this way I know it would be correct
- Do the RAUW
- If the replaced value is owned, check if it's trivially dead now
- If it's not trivially dead, insert the destroy
There was a problem hiding this comment.
Sure. I am going to do something sort of like this. One issue is that we do not know until we have processed all of the results if the overall multiple value instruction will /actually/ be dead. I am going to try restructuring the loop here to enable me to do that.
|
Yes. I am actually going to fix that a different way. I want to get rid of that code path entirely. Why can't we just add the these instructions to the worklist and visit them recursively? I find it to be hard to trigger this code path in SIL. |
|
So I took a bit more of a look at this. I was just trying to quickly fix this at the time, so I didn't meditate on the IR. The destructure simplifying code was not intended to ever work on non-trivial types that were not guaranteed. This was done since the code did not put in destroy_value. The bug is that I missed a case: |
… where we have a non-trivial value with .none ownership. In certain cases, in OSSA, non-trivial values with .none ownership can be merged into .owned aggregates such that when we extract the value back out from the aggregate we lost the information that the original value was not .owned. As an example of this consider the following SIL: bb0(%0 : @owned Builtin.NativeObject): %1 = enum $Optional<Builtin.NativeObject>, #Optional.none!enumelt %2 = tuple(%0 : $Builtin.NativeObject, %1 : $Optional<Builtin.NativeObject>) (%3, %4) = destructure_tuple %2 : $(Builtin.NativeObject, Optional<Builtin.NativeObject>) In this case, %4 has .owned ownership, while %1 has .none ownership. This is because we have lost the refined information that we originally had a .none value as input to the tuple we are destructuring. Due to this when we RAUW, we would need to insert a destroy on %4 to make sure that we maintain ossa invariants. This is safe since the destroy_value will be on a dynamically .none value, which is a no-op. That being said, the intention here was actually not to implement this pattern in the code (as can be seen by not handling @owned destructures). Thus this commit just makes the constant folding code more conservative to ensure that we do not try to handle this case.
gottesmm
changed the title
|
I am going to use this PR to land the other fix I mentioned above. |
gottesmm
force-pushed
the
pr-6a6eeddd13622a50c612240710f0e66c4e0d528b
branch
from
766ff33 to
719ee25
Compare
|
@swift-ci smoke test and merge |
3 similar comments
|
@swift-ci smoke test and merge |
|
@swift-ci smoke test and merge |
|
@swift-ci smoke test and merge |
|
@shahmishal looks like ci.swift.org is down. |
|
@swift-ci smoke test and merge |
3 similar comments
|
@swift-ci smoke test and merge |
|
@swift-ci smoke test and merge |
|
@swift-ci smoke test and merge |
Consider the following SIL:
In this case, we clearly can replace all uses of %86 with %83. But since %86 is
a multiple value instruction result, despite our RAUWing, we can not eliminate
it. This implies since we have erased %83 being .none when passing it as an
argument to %84, we need to insert a destroy.
This will eventually be a no-op in the pipeline when the destructure is torn
apart by later passes.
In order to prevent constant propagation from introducing an infinite loop due
to the insertion of these destroys, I included a set in the constant folder to
ensure that we do not revisit results that have only constant folder inserted
destroy uses.