search

swiftlang / swift

The Swift Programming Language
https://swift.org
Apache License 2.0
67.64k stars 10.38k forks source link

[SR-14088] Re-enable SIL verification for `differentiable_function` and SIL diff witnesses #54770

Open dan-zheng opened 4 years ago

dan-zheng commented 4 years ago
Previous ID SR-14088
Radar rdar://problem/73507243
Original Reporter @dan-zheng
Type Sub-task
Status In Progress
Resolution
Additional Detail from JIRA | | | |------------------|-----------------| |Votes | 0 | |Component/s | Compiler | |Labels | Sub-task | |Assignee | @rxwei | |Priority | Medium | md5: 3d2c6f9bc1f6178b50b1c770758eb175

Parent-Task:

blocks:

relates to:

Issue Description:

rxwei commented 3 years ago

@dan-zheng@marcrasi What remains to be done before they can be re-enabled?

dan-zheng commented 3 years ago

The first step would be to uncomment SILVerifier::checkDifferentiableFunctionInst: https://github.com/apple/swift/blob/27ece6f83db6bea5ee35a560c673c8eab415d1e3/lib/SIL/Verifier/SILVerifier.cpp#L4801

The verification condition for expected/actual JVP/VJP operand types can be relaxed to sth like checking for "function type ABI differences".

Re-enabling verification would be great. I think it may expose some latent issues in autodiff tests (like test/AutoDiff/reabstraction.swift) that cause flaky test failures.

Currently, @differentiable function types in SIL are quite difficult to correctly reabstract and transform because expected JVP/VJP types are computed from the original function type. I think your old idea of "creating a new SILDifferentiableFunctionType" where all three types are distinct could still be a good idea. That would also enable optimization/specialization of the original function operand when the JVP/VJP operands cannot be optimized.

rxwei commented 3 years ago

Thanks for the context. Disabling verification seemed like an suboptimal solution. There are a lot of weaker things that can be checked here, e.g. the equality of unsubstituted types or at least making sure the indirectness and ownership conventions are the same. Not checking these will lead to memory leaks (which happened to me when I make ownership-related changes) and other errors that are hard to debug.

rxwei commented 3 years ago

@swift-ci create