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

[SR-14042] Fix differentiable function reabstraction and re-enable tests #56433

Open
dan-zheng opened this issue Jan 13, 2021 · 7 comments
Open
Assignees
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself

Comments

@dan-zheng
Copy link
Collaborator

Previous ID SR-14042
Radar rdar://problem/73162805
Original Reporter @dan-zheng
Type Bug
Additional Detail from JIRA
Votes 1
Component/s Compiler
Labels Bug
Assignee @rxwei
Priority Medium

md5: 1d4a7be8650ea72475f9157c628e8d7d

relates to:

  • SR-14088 Re-enable SIL verification for differentiable_function and SIL diff witnesses
  • TF-851 [AD] Robust SILGen thunking for @differentiable functions

Issue Description:

There are runtime crashes related to differentiable function reabstraction in SIL not working properly. These issues aren't caught in SIL verification because `@differentiable`-function-type-related instruction verification is also disabled: TF-1197.

Some related tests are disabled in #35384 This issue tracks landing a robust fix for the reabstraction-related issues and re-enabling the tests.

@dan-zheng
Copy link
Collaborator Author

cc @rxwei @marcrasi

@dan-zheng
Copy link
Collaborator Author

I'll re-pitch the idea of creating a top-level SILDifferentiableFunctionType with explicit component original & JVP & VJP SILFunction types.

This representation is much less memory-efficient and harder to parse and printing. But it is explicit, so all of these reabstraction and "conservation" and phase ordering (commutative diagram) issues.

The current representation implicitly infers JVP & VJP function types (particularly the differential and pullback types) from the original function type's parameter and result conventions. It may be possible to fix all the bugs here: so that the inference is consistent everywhere, respecting phase ordering, working for both differentiable_function construction and differentiable_function_extract extraction. But that seems like a lot of work, and I'm not confident it will be clean (hack-free) and easy to understand. For example, function-type optimizations (on @differentiable closure values in SIL) may need to be conservative to prevent invariants about the bundled types from being broken.

@rxwei
Copy link
Member

rxwei commented Jan 13, 2021

@swift-ci create

@rxwei
Copy link
Member

rxwei commented Jan 13, 2021

IMO that is an overly heavyweight solution and will explore other possible approaches.

@dan-zheng
Copy link
Collaborator Author

That's fair. Look forward to more elegant ideas, happy to discuss!

@dan-zheng
Copy link
Collaborator Author

I think the lightweighter version of the idea above is storing TangentVector parameter and result conventions for pullback (and maybe also differential) functions in SIL @differentiable function types. It's these conventions that cannot be reliably inferred - no need to store extra parameter and result types.

@rxwei
Copy link
Member

rxwei commented Jan 18, 2021

Yes, that's exactly what I had planned to try. Currently we have SILParameterDifferentiability and SILResultDifferentiability for each function type parameter and result. We can compress differentiability and parameter/result convention into a single word.

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior. Also: expected but undesirable behavior. compiler The Swift compiler in itself
Projects
None yet
Development

No branches or pull requests

2 participants