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

[AutoDiff] Fix forward-mode crashes related to tangent buffers #33633

Merged
merged 11 commits into from Sep 9, 2020

Conversation

efremale
Copy link
Collaborator

@efremale efremale commented Aug 25, 2020

Fixes TF-984, SR-13447.

Fixes foward-mode crashed related to:

  • Missing tangent buffers for non-wrt inout parameters
  • Tangent buffers not being initialized due to the corresponding
    original buffer intialization instructions being non-active
  • Non-varied indirect results not being initialized
  • emitDestroyValue crashes due to TangentVector value category mismatch

lib/SILOptimizer/Differentiation/JVPCloner.cpp Outdated Show resolved Hide resolved
@@ -85,10 +85,8 @@ func activeInoutParamControlFlow(_ array: [Float]) -> Float {
return result
}

// FIXME(TF-984): Forward-mode crash due to unset tangent buffer.

This comment was marked as outdated.

Copy link
Member

@rxwei rxwei left a comment

Choose a reason for hiding this comment

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

"Function-local allocations" were introduced in pullback generation so that adjoint buffers would dominate all subsequent use sites in the reverse CFG. I don't see how they are needed in differential generation since the CFG is almost the same as the original function, so all tangent values we emit should have the same scope as their corresponding original value.

@rxwei
Copy link
Member

rxwei commented Aug 25, 2020

To me there seems to be a simpler, spot-on fix: For any non-wrt inout parameter that you treat as the result, create a tangent buffer in the beginning of the function. The "function-local allocation" fix applied in this patch seems like a big overkill because no other case would need function local allocations.

@efremale efremale marked this pull request as draft August 26, 2020 18:38
@efremale efremale force-pushed the fix-TF-984 branch 2 times, most recently from ea1a722 to 5c47a53 Compare August 27, 2020 23:36
@efremale efremale changed the title [AutoDiff] Fix forward mode differentiation for non-wrt inout parameters [AutoDiff] Fix forward-mode crashes related to tangent buffers Aug 27, 2020
Fixes TF-984, SR-13447.

Fixes foward-mode crashed related to:
- Missing tangent buffers for non-wrt `inout` parameters
- Tangent buffers not being initialized due to the corresponding
original buffer intialization instructions being non-active
- Non-varied indirect results not being initialized
- `emitDestroyValue` crashes due to `TangentVector` value category mismatch
lib/SILOptimizer/Differentiation/JVPCloner.cpp Outdated Show resolved Hide resolved
lib/SILOptimizer/Differentiation/JVPCloner.cpp Outdated Show resolved Hide resolved
lib/SILOptimizer/Differentiation/JVPCloner.cpp Outdated Show resolved Hide resolved
@efremale efremale requested a review from rxwei August 29, 2020 13:12
…BufferInitializationInfo`, make `initInfo` non-optional in `setTangentBuffer`
Copy link
Member

@rxwei rxwei left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this and addressing the comments!

Copy link
Collaborator

@dan-zheng dan-zheng left a comment

Choose a reason for hiding this comment

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

Great fixes!

lib/SILOptimizer/Differentiation/JVPCloner.cpp Outdated Show resolved Hide resolved
@dan-zheng
Copy link
Collaborator

@swift-ci Please test

@swift-ci
Copy link
Collaborator

Build failed
Swift Test Linux Platform
Git Sha - c8cb01ada4896f515ae93a75cf64188419d23991

@swift-ci
Copy link
Collaborator

Build failed
Swift Test OS X Platform
Git Sha - c8cb01ada4896f515ae93a75cf64188419d23991

…isiting all (un)initializing instructions where at least one parameter is active

We found out that it is very untrivial to correctly check initializedness of code with aggregate projections
(e.g. nested structs with multiple `struct_element_addr` referring to the same element). Instead of checking
initializedness, we can rely on correctness of the original function SIL and reproduce (un)initializing
instructions based on the original SIL.
Stylistic edits.
Fix test by removing invalid `@_silgen_name` usage.
dan-zheng and others added 3 commits September 3, 2020 23:28
End-to-end forward-mode tests should be put in test/AutoDiff/validation-test.
The forward-mode differentiation changes do not fix all crashers.
Iterate over original differentiability result indices instead of directly
iterating over original indirect results and non-wrt `inout` parameters, which
may include results that are not relevant for differentiation.

Other cleanup included.
@dan-zheng
Copy link
Collaborator

@swift-ci Please test

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

4 participants