Skip to content

Add ASTScope support for @differentiable attribute. #27451

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 2 commits into from
Oct 1, 2019

Conversation

dan-zheng
Copy link
Contributor

@dan-zheng dan-zheng commented Oct 1, 2019

@differentiable attribute where clauses may refer to generic
parameters from some generic context.

Without special ASTScope support for @differentiable attributes,
ASTScopeLookup.cpp logic tries to resolve the generic parameter
DeclNames in the where clause based on source location alone
(ASTScopeImpl::findChildContaining) and fails.

The fix is to add a special DifferentiableAttributeScope, mimicking
SpecializeAttributeScope. Every @differentiable attribute has its
own scope, derived from the declaration on which it is declared.
Unlike @_specialize, @differentiable may also be declared on
AbstractStorageDecl declarations (subscripts and variables).

Resolves TF-815.


Decl::getSourceRangeIncludingAttrs should not consider implicit
@differentiable attributes generated during @differentiating
attribute type-checking.

TF-835 tracks robust lowering for @differentiating attributes
that does not involve generating implicit @differentiable attributes,
circumventing this issue.


Unblocks swift-DEVELOPMENT-SNAPSHOT-2019-09-24-a merge into tensorflow.

`@differentiable` attribute where clauses may refer to generic
parameters from some generic context.

Without special ASTScope support for `@differentiable` attributes,
ASTScopeLookup.cpp logic tries to resolve the generic parameter
DeclNames in the where clause based on source location alone
(`ASTScopeImpl::findChildContaining`) and fails.

The fix is to add a special `DifferentiableAttributeScope`, mimicking
`SpecializeAttributeScope`. Every `@differentiable` attribute has its
own scope, derived from the declaration on which it is declared.
Unlike `@_specialize`, `@differentiable` may also be declared on
`AbstractStorageDecl` declarations (subscripts and variables).

Resolves TF-815.
`Decl::getSourceRangeIncludingAttrs` should not consider implicit
`@differentiable` attributes generated during `@differentiating`
attribute type-checking.

TF-835 tracks robust lowering for `@differentiating` attributes
that does not involve generating implicit `@differentiable` attributes,
circumventing this issue.
@dan-zheng dan-zheng added the tensorflow This is for "tensorflow" branch PRs. label Oct 1, 2019
@dan-zheng
Copy link
Contributor Author

cc @davidungar: your review would be appreciated!

@differentiable attribute requires special ASTScope support because it may contain a where clause, similar to @_specialize:

@differentiable(where T: Differentiable)
func id<T>(_ x: T) -> T { x }

I mostly copied logic from SpecializeAttributeScope. One difference is that @differentiable attribute may also be declared on AbstractStorageDecl declarations - see tests.

@dan-zheng
Copy link
Contributor Author

@swift-ci Please test tensorflow

if ((false)) {
// if (useASTScopesForLookup()) {
// SWIFT_ENABLE_TENSORFLOW END
if (useASTScopesForLookup()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: this reverts an earlier workaround hack.

@dan-zheng
Copy link
Contributor Author

dan-zheng commented Oct 1, 2019

There's one remaining test failure, which is known and orthogonal.

Failing Tests (1):
    Swift(linux-x86_64) :: AutoDiff/transposing_attr_type_checking.swift

  Expected Passes    : 10936
  Expected Failures  : 25
  Unsupported Tests  : 1366
  Unexpected Failures: 1

EDIT: macOS has one additional failure:

Failing Tests (2):
    Swift(macosx-x86_64) :: SILOptimizer/OSLogConstantEvaluableTest.swift
    Swift(macosx-x86_64) :: AutoDiff/transposing_attr_type_checking.swift

SILOptimizer/OSLogConstantEvaluableTest.swift failure is expected because -enable-ownership-stripping-after-serialization has been disabled on tensorflow branch due to SR-11336. Disabled in #27453.

@dan-zheng dan-zheng requested review from rxwei and marcrasi October 1, 2019 02:32
Copy link

@burmako burmako left a comment

Choose a reason for hiding this comment

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

Thank you for the detailed comments! From what I can see, this looks reasonable.

@dan-zheng dan-zheng merged commit e254b29 into swiftlang:tensorflow-merge Oct 1, 2019
@dan-zheng dan-zheng deleted the tensorflow-merge branch October 1, 2019 03:17
dan-zheng added a commit to dan-zheng/swift that referenced this pull request Jan 13, 2020
`@differentiable` attributes may contain `where` clauses referencing generic
parameters from some generic context, just like `@_specialize` attributes.

Without special ASTScope support for `@differentiable` attributes,
ASTScopeLookup.cpp logic tries to resolve the generic parameter `DeclName`s in
`where` clauses based on source location alone
(`ASTScopeImpl::findChildContaining`) and fails.

The fix is to add a special `DifferentiableAttributeScope`, mimicking
`SpecializeAttributeScope`. Every `@differentiable` attribute has its own scope,
derived from the declaration on which it is declared. Unlike `@_specialize`,
`@differentiable` may also be declared on `AbstractStorageDecl` declarations
(subscripts and variables).

Upstreams swiftlang#27451.

Progress towards TF-828: upstream `@differentiable` attribute type-checking.
dan-zheng added a commit that referenced this pull request Jan 14, 2020
`@differentiable` attributes may contain `where` clauses referencing generic
parameters from some generic context, just like `@_specialize` attributes.

Without special ASTScope support for `@differentiable` attributes,
ASTScopeLookup.cpp logic tries to resolve the generic parameter `DeclName`s in
`where` clauses based on source location alone
(`ASTScopeImpl::findChildContaining`) and fails.

The fix is to add a special `DifferentiableAttributeScope`, mimicking
`SpecializeAttributeScope`. Every `@differentiable` attribute has its own scope,
derived from the declaration on which it is declared. Unlike `@_specialize`,
`@differentiable` may also be declared on `AbstractStorageDecl` declarations
(subscripts and variables).

Upstreams #27451.

Progress towards TF-828: upstream `@differentiable` attribute type-checking.
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.

4 participants