[AutoDiff] fix ownership instructions (SR-13973) #35196
Conversation
|
@swift-ci please test |
|
|
||
| struct SR13973 { | ||
| let x: Float = 0 | ||
|
|
There was a problem hiding this comment.
For changes like this, both are important!
There was a problem hiding this comment.
I added a FileCheck to test/AutoDiff/SILOptimizer/differentiation_function_canonicalization.sil. It's pretty much the same thing that I do in this file, except in SIL.
marcrasi
force-pushed
the
marcrasi-fix-sr13973
branch
from
1640d0a to
38b5112
Compare
| // CHECK: [[CURRIED_JVP:%.*]] = partial_apply [callee_guaranteed] [[METHOD_JVP]]([[SELF]]) : $@convention(thin) (Float, S) -> (Float, @owned @callee_guaranteed (Float) -> Float) | ||
| // CHECK: [[METHOD_VJP:%.*]] = differentiability_witness_function [vjp] [parameters 0] [results 0] @method_thin : $@convention(thin) (Float, S) -> Float | ||
| // CHECK: [[CURRIED_VJP:%.*]] = partial_apply [callee_guaranteed] [[METHOD_VJP]]([[SELF]]) : $@convention(thin) (Float, S) -> (Float, @owned @callee_guaranteed (Float) -> Float) | ||
| // CHECK: strong_retain [[CURRIED]] : $@callee_guaranteed (Float) -> Float |
There was a problem hiding this comment.
This strong_retain gets added by this PR.
|
|
||
| struct SR13973 { | ||
| let x: Float = 0 | ||
|
|
There was a problem hiding this comment.
I added a FileCheck to test/AutoDiff/SILOptimizer/differentiation_function_canonicalization.sil. It's pretty much the same thing that I do in this file, except in SIL.
| return %5 : $() | ||
| } | ||
|
|
||
| // CHECK-LABEL: sil {{.*}} @AD__method_curried__differentiable_curry_thunk_src_0_wrt_0 |
There was a problem hiding this comment.
Can you add a ':' at the end of this to prevent prefix double matches?
Can you also add an } // end sil function '$NAME' to the end.
This is just basic SIL test hygiene.
|
|
||
| // CHECK-LABEL: sil {{.*}} @AD__method_curried__differentiable_curry_thunk_src_0_wrt_0 | ||
| // CHECK: bb0([[SELF:%.*]] : $S): | ||
| // CHECK: [[METHOD:%.*]] = function_ref @method_thin : $@convention(thin) (Float, S) -> Float |
There was a problem hiding this comment.
Can you add an ossa test? Doesn't this transform only run on OSSA? I guess not?
There was a problem hiding this comment.
Does that mean add a test where the sil functions say [ossa]? Done.
I guess this transform doesn't only run on OSSA...
I don't really understand what I am doing, but my reasoning is: #34693 made
getOwnershipKind()start returningNonewhen the function doesn't have ownership, and this caused Differentiation to insert fewer copy value operations, which caused https://bugs.swift.org/browse/SR-13973. So modify the condition to insert copy value operations when the function doesn't have ownership.Only the changes on L503 and L1217 are covered by the new test, but I changed the other similar-looking conditions too, because it seems like they probably need the same change.