Skip to content

[AutoDiff] Support inout argument differentiation. #30013

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

Merged
merged 8 commits into from
Feb 23, 2020

Conversation

dan-zheng
Copy link
Contributor

@dan-zheng dan-zheng commented Feb 22, 2020

Includes cherry-pick of #29959: typing rules for inout parameter differentiation.


Add reverse-mode differentiation support for apply with inout arguments.

Notable pullback generation changes:

  • If the pullback seed argument is inout, assign it (rather than a copy)
    directly as the adjoint buffer of the original result. This is important so
    the value is updated in-place.
  • In visitApplyInst: skip adjoint accumulation for inout arguments.
    Adjoint accumulation for inout arguments occurs when callee pullbacks are
    applied, so no extra accumulation is necessary.

Add derivatives for functions with inout parameters in the stdlib for testing:

  • FloatingPoint operations: +=, -=, *=, /=
  • Array.append

Resolves TF-1165.

Todos:

  • Add more tests, e.g. SILGen tests for inout derivative typing rules.
  • Evaluate performance of inout derivatives vs functional derivatives + mutation.
  • TF-1166: enable @differentiable attribute on set accessors.
  • TF-1173: add forward-mode differentiation support for apply with inout
    parameters.

Exposes TF-1175: incorrect activity for class arguments.
Exposes TF-1176: incorrect activity for class modify accessors.
Add negative tests.


Examples:

// Test `inout` identity function.
func inoutIdentity(_ x: inout Float) {}
func identity(_ x: Float) -> Float {
  var result = x
  inoutIdentity(&result) // here
  return result
}
print(valueWithGradient(at: 10, in: identity))
// (value: 10.0, gradient: 1.0)
// Test `Float.*=` and control flow differentiation.
func product(_ array: [Float]) -> Float {
  var result: Float = 1
  for i in withoutDerivative(at: array.indices) {
    result *= array[i] // here
    // Old functional version:
    // result = result * array[i]
  }
  return result
}
print(valueWithGradient(at: [3, 4, 5], in: product))
// (value: 60.0, gradient: [20.0, 15.0, 12.0])
// Test `Array.append` and control flow differentiation.
func squared(_ array: [Float]) -> [Float] {
  var results: [Float] = []
  for i in withoutDerivative(at: array.indices) {
    results.append(array[i] * array[i]) // here
    // Old functional version:
    // results = results + [array[i] * array[i]]
  }
  return results
}
let v: Array<Float>.TangentVector = [4, -5, 6]
let (value, pb) = valueWithPullback(at: [1, 2, 3], in: squared)
print(value, pb(v))
// [1.0, 4.0, 9.0] [8.0, -20.0, 36.0]
// Test `set` accessor differentiation.
struct S: Differentiable {
  var x: Float

  var computed: Float {
    get { x }
    set { x = newValue }
  }
}
func squared(_ x: Float) -> Float {
  var s = S(x: 1)
  s.x *= x // here
  s.computed *= x // here
  return s.x
}
print(valueWithGradient(at: 10, in: squared))
// (value: 100.0, gradient: 20.0)

Thanks @marcrasi for peer debugging incorrect inout argument derivative values!

We discovered that Pullback::visitApplyInst should skip adjoint accumulation for inout arguments, which is interesting and not obvious. This made the first exciting test cases pass.

Working together was fun and productive 🙂

@dan-zheng dan-zheng added the tensorflow This is for "tensorflow" branch PRs. label Feb 22, 2020
@dan-zheng dan-zheng requested review from rxwei and marcrasi February 22, 2020 20:25
@dan-zheng
Copy link
Contributor Author

Note: merging master branch PR #29959 (inout differentiation typing rules) to tensorflow branch requires this PR, which has non-trivial accompanying changes to fix tests:

Failing Tests (5):
    Swift(linux-x86_64) :: AutoDiff/downstream/activity_analysis.swift
    Swift(linux-x86_64) :: AutoDiff/downstream/differentiation_transform_diagnostics.swift
    Swift(linux-x86_64) :: AutoDiff/downstream/differentiable_attr_type_checking.swift
    Swift(linux-x86_64) :: AutoDiff/downstream/forward_mode_diagnostics.swift
    Swift(linux-x86_64) :: AutoDiff/compiler_crashers_fixed/tf1167-differentiable-attr-override-match.swift

This is blocking the master -> tensorflow merge, so I'd like to land this PR as soon as possible.

Lesson learned: next time when doing changes that touch both upstream and downstream code, create PRs to both tensorflow and master branch, and test+merge the tensorflow branch PR first. This makes the master -> tensorflow merge process much easier.

@saeta
Copy link
Contributor

saeta commented Feb 22, 2020

🎉

Semantically, an `inout` parameter is both a parameter and a result.

`@differentiable` and `@derivative` attributes now support original functions
with one "semantic result": either a formal result or an `inout` parameter.

Derivative typing rules for functions with `inout` parameters are now defined.

The differential/pullback type of a function with `inout` differentiability
parameters also has `inout` parameters. This is ideal for performance.

Differential typing rules:
- Case 1: original function has no `inout` parameters.
  - Original:     `(T0, T1, ...) -> R`
  - Differential: `(T0.Tan, T1.Tan, ...) -> R.Tan`
- Case 2: original function has a non-wrt `inout` parameter.
  - Original:     `(T0, inout T1, ...) -> Void`
  - Differential: `(T0.Tan, ...) -> T1.Tan`
- Case 3: original function has a wrt `inout` parameter.
  - Original:     `(T0, inout T1, ...) -> Void`
  - Differential: `(T0.Tan, inout T1.Tan, ...) -> Void`

Pullback typing rules:
- Case 1: original function has no `inout` parameters.
  - Original: `(T0, T1, ...) -> R`
  - Pullback: `R.Tan -> (T0.Tan, T1.Tan, ...)`
- Case 2: original function has a non-wrt `inout` parameter.
  - Original: `(T0, inout T1, ...) -> Void`
  - Pullback: `(T1.Tan) -> (T0.Tan, ...)`
- Case 3: original function has a wrt `inout` parameter.
  - Original: `(T0, inout T1, ...) -> Void`
  - Pullback: `(inout T1.Tan) -> (T0.Tan, ...)`

Resolves TF-1164.
@dan-zheng dan-zheng force-pushed the autodiff-inout-parameters branch from a1ed40c to 759f338 Compare February 22, 2020 21:14
@dan-zheng dan-zheng changed the title [AutoDiff] Support differentiation of apply with inout arguments. [AutoDiff] Support inout argument differentiation. Feb 22, 2020
Add reverse-mode differentiation support for `apply` with `inout` arguments.

Notable pullback generation changes:
- If the pullback seed argument is `inout`, assign it (rather than a copy)
  directly as the adjoint buffer of the original result. This is important so
  the value is updated in-place.
- In `visitApplyInst`: skip adjoint accumulation for `inout` arguments.
  Adjoint accumulation for `inout` arguments occurs when callee pullbacks are
  applied, so no extra accumulation is necessary.

Add derivatives for functions with `inout` parameters in the stdlib for testing:
- `FloatingPoint` operations: `+=`, `-=`, `*=`, `/=`
- `Array.append`

Resolves TF-1165.

Todos:
- Add more tests, e.g. SILGen tests for `inout` derivative typing rules.
- Evaluate performance of `inout` derivatives vs functional derivatives + mutation.
- TF-1166: enable `@differentiable` attribute on `set` accessors.
- TF-1173: add forward-mode differentiation support for `apply` with `inout`
  parameters.
@dan-zheng dan-zheng force-pushed the autodiff-inout-parameters branch from 759f338 to f1a604d Compare February 22, 2020 22:06
Copy link
Contributor

@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.

Missing test cases:

  1. Differentiating a function with inout parameters that have a class type. This is expected to work normally.
  2. Differentiating a function whose body calls a same-file function that both takes a Differentiable inout argument and returns a Differentiable formal result where both the inout argument and the formal result are active. This is expected to trigger a differentiation error.

Fixes `-O` test failure:

```
Failing Tests (1):
    Swift(linux-x86_64) :: AutoDiff/downstream/inout_parameters.swift
```
Class-typed arguments are not always marked active.
This produces incorrect derivative results.
Mutating `inout` class argument via `store`: correct derivatives.

Mutating `inout` class argument via `modify` accessor: incorrect derivatives.
Tracked at TF-1176.
@dan-zheng dan-zheng force-pushed the autodiff-inout-parameters branch from 398c13f to 955b594 Compare February 23, 2020 02:32
Copy link
Contributor

@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.

Any forward mode tests?

@dan-zheng
Copy link
Contributor Author

Any forward mode tests?

Forward-mode differentiation doesn't support inout arguments yet: TF-1173.
Adding support should involve straightforward changes to JVPEmitter::visitApplyInst.
In the meantime, forward-mode inout argument differentiation remains diagnosed.

Let's standardize on `unsigned`.
@dan-zheng
Copy link
Contributor Author

@swift-ci Please test tensorflow

@dan-zheng dan-zheng merged commit dd1d689 into swiftlang:tensorflow Feb 23, 2020
@dan-zheng dan-zheng deleted the autodiff-inout-parameters branch February 23, 2020 14:24
OpenSpiel pushed a commit to google-deepmind/open_spiel that referenced this pull request Feb 26, 2020
Change the following:
- `ActionValueCalculator` from a struct to a class.
- `ActionValueCalculator.loss` from a mutating method (necessary for structs) to
  a non-mutating one (unnecessary for classes).

This fixes a new type-checking error after `inout` argument differentiation
typing rules, added in swiftlang/swift#30013:
```
ExploitabilityDescent.swift:218:4:
error: cannot differentiate between functions with both an 'inout' parameter and a result
  @differentiable(wrt: policy)
```
PiperOrigin-RevId: 297457176
Change-Id: I3374d45c1d56124e20973a7f262bd10bd6e06053
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tensorflow This is for "tensorflow" branch PRs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants