[sil] Add a new CastConsumptionKind called BorrowAlways. #19772
Conversation
|
@swift-ci smoke test |
|
Forgot to mention, I also changed all '==' queries on CastConsumptionKind to instead use covered switches. |
|
The current precondition on using the scalar cast instructions isn't just that the types are address-only; they require the target value to be a directly-stored part of the source value, i.e. a class upcast/downcast or an existential value or something like that. The general cast functions can do a lot of things that produce owned values that are unrelated, ownership-wise, to the source values; and they can do this even when the types involved are both loadable (e.g. if you're casting an |
|
@rjmccall in that situation, we do a copy and then borrow. Just like we already do today except that today we do a copy + pass it down as take always. |
|
@rjmccall sorry I may have spoken too fast about the copy + take_always (I remember a piece of code that did something like that, but I can't seem to find it now). That being said, I think if we know that we are going to need to go through memory to case then we can copy if we need to before, run it through as +1 and then borrow again. Or if we do not need to copy and the cast machinery will do the copy for us and return a +1 value, then we just borrow afterwards. |
|
So is this not a valid alternative on |
|
Yes. I even put in an assert to make that clear. |
|
Look at the constructor of CheckedCastAddrBranch. |
|
Okay. Well, please at least fix the comments to explain what I said, then. Also, given that we're moving towards opaque values, is it a good idea to include a feature that can't be reliably lowered to addresses? |
|
I haven't talked to Andy about this, but I imagine that all of the borrow stuff could become copy_on_success. But I haven't thought in depth (trying to finish that proposal to get to you). @atrick your thoughts? |
|
I don't want to hold this PR up. I think that the overall simplest form of the cast is |
|
@atrick I am fine with that. I do think that SILGen needs to put in those copies initially. Like you said, I definitely think we are going to want to have an optimization that changes +0 pattern matching into +1 pattern matching if a value is consumed. |
This means that: 1. SILGenPattern always borrows the object before it emits a case. 2. Any cast with this cast has a +0 result. NOTE: That one can not use this with address types (so we assert if you pass this checked_cast_addr_br). NOTE: Once we have opaque values, checked_cast_br of a guaranteed value will lower to a copy + checked_cast_addr_br (assuming the operation is a consuming cast). To make sure this does not become a problem in terms of performance, we will need a pass that can transform SILGenPattern +0 cases to +1 cases. This is something that we have talked about in the past and I think it is reasonable to implement. This is an incremental commit towards fixing SILGenPattern for ownership. rdar://29791263
gottesmm
force-pushed
the
pr-a6c84965d1903f06da9c5c62ff4b58d915cc4807
branch
from
9fc6f31 to
62b5110
Compare
|
@swift-ci smoke test and merge |
|
@swift-ci smoke test linux platform |
1 similar comment
|
@swift-ci smoke test linux platform |
|
@swift-ci test linux platform |
|
@rjmccall @atrick I was thinking about this last night and I remembered more. Basically I don't think we need this CastConsumptionKind long term. The reason I am adding this is to enable me to borrow inside SILGenPattern since SILGenPattern uses CastConsumptionKind via ConsumableManagedValue to control the ownership of what it emits. Once I am finished changing SILGenPattern to emit code at +0 I think that we will be able to change SILGenPattern to just work with ManagedValues instead of ConsumableManagedValue and then get rid of this extra case. |
|
👍 |
This means that:
NOTE: That one can not use this with address only types (so we assert if you
pass this to any of the checked cast instructions).
This is an incremental commit towards fixing SILGenPattern for ownership.
rdar://29791263
Just to be clear this doesn't change anything to actually use BorrowAlways.