[AutoDiff] Update @differentiable attribute, refactor `lookupFuncDecl` method. #17246

dan-zheng · 2018-06-15T17:38:10Z

Add adjoint access level check to @differentiable attribute.
- If the original function is "exported" (public or @_versioned), then the
  adjoint must also be exported.
Generalize lookupFuncDecl function and add it as a method to TypeChecker
so that it can be used for type-checking #adjoint expression in a later
commit.

@differentiable

…` method. - Add adjoint access level check to @differentiable attribute. - If the original function is "exported" (public or @_versioned), then the adjoint must also be exported. - Generalize `lookupFuncDecl` function and add it as a method to `TypeChecker` so that it can be used for type-checking #adjoint expression in a later commit.

rxwei · 2018-06-15T17:59:30Z

lib/Sema/TypeCheckExpr.cpp

+  }
+  // If function declaration could not be resolved, emit the appropriate
+  // diagnostic.
+  if (!resolvedFuncDecl) {


Flip this and early return?

rxwei · 2018-06-15T18:00:38Z

lib/Sema/TypeCheckAttr.cpp

+    std::function<void()> primalInvalidTypeContextDiagnostic = [&]() {
+      TC.diagnose(primalNameLoc,
+                  diag::differentiable_attr_function_not_same_type_context,
+                  primalSpecifier.Name);


Do you need a setInvalid here?

I think setInvalid is handled by the block below:

primal = TC.lookupFuncDecl(...) if (!primal) { attr->setInvalid(); return; }

If any diagnostic is emitted, primal will be nullptr and attr->setInvalid() will execute.

rxwei · 2018-06-15T18:08:14Z

lib/Sema/TypeCheckExpr.cpp

+      notAFuncDecl = true;
+      continue;
+    }
+    if (hasValidTypeCtx.hasValue()) {


if (hasValidTypeCtx && !(*hasValidTypeCtx)(funcDecl)) { wrongTypeContext = true; continue; }

rxwei · 2018-06-15T18:10:05Z

lib/Sema/TypeCheckExpr.cpp

+  // If function declaration could not be resolved, emit the appropriate
+  // diagnostic.
+  if (!resolvedFuncDecl) {
+    if (results.empty()) {


Instead of these if-elses, I would suggest

if (results.empty()) { diagnose(...) return; } if (ambiguousFuncDecl) { ambiguousDiagnostic(); return; } ...

rxwei · 2018-06-15T18:11:20Z

lib/Sema/TypeCheckExpr.cpp

+  // the type context.
+  // This works around the fact that operators cannot be specified with a
+  // qualified name (i.e. only `(+)` works, `Float.(+)` doesn't).
+  if (funcName.isOperator() && lookupContext->isTypeContext()) {


How about

if (funcName.isOperator() && lookupContext->isTypeContext()) if (auto tmp = lookupMember(lookupContext, lookupContext->getSelfTypeInContext(), funcName)) results = tmp;

Before writing any empty() checks, I'd usually check whether tmp has operator bool() first. In this case LookupResult does have one.

Thanks for the tip! Done.

@rxwei

Addresses comments from @rxwei.

dan-zheng

Thanks! I'll add tests for @differentiable access control before merging.

dan-zheng · 2018-06-15T18:19:30Z

lib/Sema/TypeCheckAttr.cpp

+    std::function<void()> primalInvalidTypeContextDiagnostic = [&]() {
+      TC.diagnose(primalNameLoc,
+                  diag::differentiable_attr_function_not_same_type_context,
+                  primalSpecifier.Name);


I think setInvalid is handled by the block below:

primal = TC.lookupFuncDecl(...) if (!primal) { attr->setInvalid(); return; }

If any diagnostic is emitted, primal will be nullptr and attr->setInvalid() will execute.

dan-zheng · 2018-06-15T18:22:48Z

lib/Sema/TypeCheckExpr.cpp

+  // the type context.
+  // This works around the fact that operators cannot be specified with a
+  // qualified name (i.e. only `(+)` works, `Float.(+)` doesn't).
+  if (funcName.isOperator() && lookupContext->isTypeContext()) {


Thanks for the tip! Done.

dan-zheng · 2018-06-15T18:23:32Z

lib/Sema/TypeCheckExpr.cpp

+      notAFuncDecl = true;
+      continue;
+    }
+    if (hasValidTypeCtx.hasValue()) {


dan-zheng · 2018-06-15T18:26:03Z

lib/Sema/TypeCheckExpr.cpp

+  }
+  // If function declaration could not be resolved, emit the appropriate
+  // diagnostic.
+  if (!resolvedFuncDecl) {


dan-zheng · 2018-06-15T18:26:07Z

lib/Sema/TypeCheckExpr.cpp

+  // If function declaration could not be resolved, emit the appropriate
+  // diagnostic.
+  if (!resolvedFuncDecl) {
+    if (results.empty()) {


@differentiable

- Add @differentiable access control check to primal as well as adjoint. - Add @differentiable access control test. - Add diagnostic for differentiating void functions.

dan-zheng · 2018-06-15T19:37:39Z

@rxwei
Could you please re-review? I made some slightly non-trivial changes in 47f49f6.

rxwei · 2018-06-15T20:40:47Z

LGTM

dan-zheng · 2018-06-15T21:11:44Z

Good idea about renaming @_versioned to @usableFromInline, done.

dan-zheng · 2018-06-15T21:16:46Z

@swift-ci Please test tensorflow

@differentiable

…` method. (#17246) * [AutoDiff] Update @differentiable attribute, refactor `lookupFuncDecl` method. - Add adjoint access level check to @differentiable attribute. - If the original function is "exported" (public or @_versioned), then the adjoint must also be exported. - Generalize `lookupFuncDecl` function and add it as a method to `TypeChecker` so that it can be used for type-checking #adjoint expression in a later commit. * Refactoring/clean up. Addresses comments from @rxwei. * Add/fix @differentiable attribute diagnostics, clean up. - Add @differentiable access control check to primal as well as adjoint. - Add @differentiable access control test. - Add diagnostic for differentiating void functions. * Preemptively rename @_versioned to @usableFromInline.

@differentiable

…` method. (#17246) * [AutoDiff] Update @differentiable attribute, refactor `lookupFuncDecl` method. - Add adjoint access level check to @differentiable attribute. - If the original function is "exported" (public or @_versioned), then the adjoint must also be exported. - Generalize `lookupFuncDecl` function and add it as a method to `TypeChecker` so that it can be used for type-checking #adjoint expression in a later commit. * Refactoring/clean up. Addresses comments from @rxwei. * Add/fix @differentiable attribute diagnostics, clean up. - Add @differentiable access control check to primal as well as adjoint. - Add @differentiable access control test. - Add diagnostic for differentiating void functions. * Preemptively rename @_versioned to @usableFromInline.

@differentiable

…` method. (#17246) * [AutoDiff] Update @differentiable attribute, refactor `lookupFuncDecl` method. - Add adjoint access level check to @differentiable attribute. - If the original function is "exported" (public or @_versioned), then the adjoint must also be exported. - Generalize `lookupFuncDecl` function and add it as a method to `TypeChecker` so that it can be used for type-checking #adjoint expression in a later commit. * Refactoring/clean up. Addresses comments from @rxwei. * Add/fix @differentiable attribute diagnostics, clean up. - Add @differentiable access control check to primal as well as adjoint. - Add @differentiable access control test. - Add diagnostic for differentiating void functions. * Preemptively rename @_versioned to @usableFromInline.

dan-zheng requested a review from rxwei June 15, 2018 17:38

rxwei reviewed Jun 15, 2018

View reviewed changes

Refactoring/clean up.

12d3a98

Addresses comments from @rxwei.

dan-zheng commented Jun 15, 2018

View reviewed changes

rxwei approved these changes Jun 15, 2018

View reviewed changes

Add/fix @differentiable attribute diagnostics, clean up.

47f49f6

- Add @differentiable access control check to primal as well as adjoint. - Add @differentiable access control test. - Add diagnostic for differentiating void functions.

Preemptively rename @_versioned to @usableFromInline.

aac118b

dan-zheng force-pushed the diff-attr-update branch from dd020fb to aac118b Compare June 15, 2018 21:14

rxwei self-assigned this Jun 15, 2018

rxwei merged commit 7bea477 into swiftlang:tensorflow Jun 15, 2018

dan-zheng added the tensorflow This is for "tensorflow" branch PRs. label Jun 15, 2018

[AutoDiff] Update @differentiable attribute, refactor lookupFuncDecl method. #17246

[AutoDiff] Update @differentiable attribute, refactor lookupFuncDecl method. #17246

Uh oh!

Conversation

dan-zheng commented Jun 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dan-zheng left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dan-zheng commented Jun 15, 2018

Uh oh!

rxwei commented Jun 15, 2018

Uh oh!

dan-zheng commented Jun 15, 2018

Uh oh!

dan-zheng commented Jun 15, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

[AutoDiff] Update @differentiable attribute, refactor `lookupFuncDecl` method. #17246

[AutoDiff] Update @differentiable attribute, refactor `lookupFuncDecl` method. #17246