Skip to content

[AutoDiff] Fix stdlib @derivative(of: subscript) crash. #28935

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 3 commits into from
Jan 3, 2020

Conversation

dan-zheng
Copy link
Contributor

@dan-zheng dan-zheng commented Dec 23, 2019

Fix @derivative(of: subscript) crash for SIMD{n} types.
Strangely, the crash is not easily reproducible via standalone tests:

// Standalone reproducer: no crash.
 extension SIMD2 {
  @differentiable(
    where Scalar : EuclideanDifferentiable & BinaryFloatingPoint,
          Scalar.TangentVector : BinaryFloatingPoint)
  public subscript(index index: Int) -> Scalar {
    @_transparent get { fatalError() }
    @_transparent set { fatalError() }
  }
}
extension SIMD2
  where Scalar : EuclideanDifferentiable & BinaryFloatingPoint,
        Scalar.TangentVector : BinaryFloatingPoint {
  @usableFromInline
  @derivative(of: subscript(index:))
  internal func _vjpSubscript(index: Int)
  -> (value: Scalar, pullback: (Scalar.TangentVector) -> TangentVector) {
    return (self[index], { v in
      var zeros = Self.zero
      zeros[index] = Scalar(v)
      return zeros
    })
  }
}

Resolves TF-1090.

Exposes TF-1094: during .swiftinterface compilation: unserialized
@derivative function is not registered for serialized @differentiable
function.


Issue originally reported by @bartchr808 in 73d43cf.

Fix `@derivative(of: subscript)` crash for SIMD{n} types.
Strangely, standalone reproducers do not crash.

Resolves TF-1090.
@dan-zheng dan-zheng added the tensorflow This is for "tensorflow" branch PRs. label Dec 23, 2019
@dan-zheng
Copy link
Contributor Author

@swift-ci Please test tensorflow

If `lookupContext` is a type context, use its `self` type for member lookup.
@dan-zheng
Copy link
Contributor Author

@swift-ci Please test tensorflow

Copy link
Contributor

@bartchr808 bartchr808 left a comment

Choose a reason for hiding this comment

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

Looks great! Looking back, making SIMD differentiable has brought to light several interesting bugs 😄

Fix validation-test/ParseableInterface/verify_stdlib.swift.

TF-1094:
- During `.swiftinterface` compilation: unserialized `@derivative` function
  is not registered for serialized `@differentiable` function.
- Specific example: original function `SIMD{n}.subscript(_:).getter` is
  serialized but derivative `SIMD{n}._vjpSubscript(_:)` is not serialized.

Workaround: mark `SIMD{n}._vjpSubscript(_:)` as `@inlinable` (serialized).

Add negative test for TF-1094.
@dan-zheng
Copy link
Contributor Author

@swift-ci Please test tensorflow

@dan-zheng dan-zheng merged commit 31c81c6 into swiftlang:tensorflow Jan 3, 2020
@dan-zheng dan-zheng deleted the TF-1090 branch January 6, 2020 05:07
dan-zheng added a commit to dan-zheng/swift that referenced this pull request Jan 6, 2020
Upstream derivative function lookup changes from
swiftlang#28935.
dan-zheng added a commit that referenced this pull request Jan 6, 2020
Upstream derivative function lookup changes from
#28935.
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