Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SILOptimizer: fix partial_apply optimization. #31552

Merged
merged 2 commits into from May 5, 2020

Conversation

dan-zheng
Copy link
Collaborator

@dan-zheng dan-zheng commented May 5, 2020

partial_apply can be rewritten to thin_to_thick_function only if the
specialized callee is @convention(thin).

This condition is newly exercised by the differentiation transform:
{JVP,VJP}Emitter::visitApplyInst generates argument-less partial_apply
with @convention(method) callees.

Re-enables tests disabled in #31545. CI breakages should be fixed.

Resolves SR-12732:

SIL verification failed: operand of thin_to_thick_function must be thin: opFTy->getRepresentation() == SILFunctionType::Representation::Thin
Verifying instruction:
     // function_ref specialized Differentiable._vjpWithDerivative(_:)
  %10 = function_ref @$s16_Differentiation14DifferentiablePAAE18_vjpWithDerivativeyx5value_13TangentVectorQzAGc8pullbacktyAGzcF0A8Unittest7TrackedVySfG_Tg5 : $@convention(method) (@guaranteed @callee_guaranteed @substituted <τ_0_0> (@inout τ_0_0) -> () for <Tracked<Float>>, @in_guaranteed Tracked<Float>) -> (@out Tracked<Float>, @owned @callee_guaranteed @substituted <τ_0_0, τ_0_1> (@in_guaranteed τ_0_0) -> @out τ_0_1 for <Tracked<Float>, Tracked<Float>>) // user: %11
->   %11 = thin_to_thick_function %10 : $@convention(method) (@guaranteed @callee_guaranteed @substituted <τ_0_0> (@inout τ_0_0) -> () for <Tracked<Float>>, @in_guaranteed Tracked<Float>) -> (@out Tracked<Float>, @owned @callee_guaranteed @substituted <τ_0_0, τ_0_1> (@in_guaranteed τ_0_0) -> @out τ_0_1 for <Tracked<Float>, Tracked<Float>>) to $@callee_guaranteed (@guaranteed @callee_guaranteed @substituted <τ_0_0> (@inout τ_0_0) -> () for <Tracked<Float>>, @in_guaranteed Tracked<Float>) -> (@out Tracked<Float>, @owned @callee_guaranteed @substituted <τ_0_0, τ_0_1> (@in_guaranteed τ_0_0) -> @out τ_0_1 for <Tracked<Float>, Tracked<Float>>) // user: %12

`partial_apply` can be rewritten to `thin_to_thick_function` only if the
specialized callee is `@convention(thin)`.

This condition is newly exercised by the differentiation transform:
`{JVP,VJP}Emitter::visitApplyInst` generates argument-less `partial_apply`
with `@convention(method)` callees.

Resolves SR-12732.
@dan-zheng dan-zheng requested review from rxwei and atrick May 5, 2020 04:24
// Do not rewrite `partial_apply` to `thin_to_thick_function` if the specialized
// callee is not `@convention(thin)`.

import DifferentiationUnittest
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be nice to remove the dependency on DifferentiationUnittest. I tried but didn't quite figure it out: -enable-library-evolution seems involved, but passing it exposes another bug (TF-1274).

Let's visit this later to unblock progress.

@dan-zheng
Copy link
Collaborator Author

@swift-ci Please test

@dan-zheng dan-zheng requested a review from marcrasi May 5, 2020 14:51
@dan-zheng
Copy link
Collaborator Author

dan-zheng commented May 5, 2020

Merging to fix CI breakages. Happy to address any feedback later!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants