[AutoDiff] declaration-only `SILDifferentiabilityWitness` #27854

marcrasi · 2019-10-23T23:57:48Z

We need declaration-only SILDifferentiabilityWitness so that we can refer to differentiability witnesses defined in other modules.

Therefore, this PR:

Adds distinct declaration/definition create methods for SILDifferentiabilityWitness. (credit to @dan-zheng's original work on this)
Handles the distinction in SIL printing & parsing: The definitions have a body in braces, and the declarations don't.
Handles the distinction in serialization: There is a new bit describing whether it's a declaration or definition.
Fixes the "deserialization currently fails if public function bodies are removed so that they are only declarations" problem noted in test/AutoDiff/sil_differentiability_witness.sil.
- The overall purpose of this PR is to allow us to have decl-only differentiability witnesses referencing decl-only functions, so this fix is important to the overall purpose of this PR.
- The change at fn = State.getGlobalNameForReference(name, fnType, fnNameLoc); in ParseSIL.cpp is what fixes this.
Adds test for a decl differentiability witness.

Todos: - Update printing/parsing - Consider safer `convertToDefinition` function, from SILWitnessTable. - I think we could defer this and start with straightforward definition/declaration constructors.

marcrasi · 2019-10-23T23:58:10Z

@swift-ci please test tensorflow

marcrasi · 2019-10-24T00:19:55Z

@swift-ci please test tensorflow macos

dan-zheng

Nice!

For reference: the compiler doesn't yet generate differentiability witnesses, that will be done by the differentiation transform later.

dan-zheng · 2019-10-24T00:25:22Z

test/AutoDiff/sil_differentiability_witness.sil

+
+sil @externalFn1 : $@convention(thin) (Float) -> Float
+
+sil @AD__externalFn1__jvp_src_0_wrt_0 : $@convention(thin) (Float) -> (Float, @owned @callee_guaranteed (Float) -> Float) {


I wonder if we want to support "differentiability witness definition (has body) referring to JVP/VJP declarations (bodiless)"?

I wonder if the test still passes if @AD__externalFn1__jvp_src_0_wrt_0 and @AD__externalFn1__vjp_src_0_wrt_0 are made bodiless?

Yes it passes. I'll add a test that does that. (Bodiless original with defined jvp/vjp seems like the most common case that will happen in practice so I want to keep a test that does it this way rather than modifying this test to have bodiless jvp/vjp).

lib/ParseSIL/ParseSIL.cpp

include/swift/SIL/SILDifferentiabilityWitness.h

rxwei

Although isDeclaration makes sense, it's a bit odd that it's not reflected in the syntax in any way. How about adding a [declaration] attribute?

dan-zheng · 2019-10-24T05:23:05Z

Although isDeclaration makes sense, it's a bit odd that it's not reflected in the syntax in any way. How about adding a [declaration] attribute?

Is there precedent for an explicit [declaration] attribute?

Syntactically, I thought "no body" implies "declaration" and "has body" implies "definition" for other SIL constructs. If we add a [declaration] attribute, do we introduce syntactically valid but semantically invalid witnesses? (examples: [declaration] witnesses with body and bodiless "no [declaration]" witnesses)

I'd prefer no [declaration] attribute unless there's a good reason to add it.

rxwei · 2019-10-24T05:32:08Z

Of course, it is unprecedented. [declaration] is just an example of what you can add to differentiate it from non-declarations. However, let's consider situations where you actually need to write SIL tests to trigger differentiation: how can the differentiation transform determine whether your test file means to declare a witness, or to create a witness that is to be canonicalized?

dan-zheng · 2019-10-24T05:46:53Z

However, let's consider situations where you actually need to write SIL tests to trigger differentiation: how can the differentiation transform determine whether your test file means to declare a witness, or to create a witness that is to be canonicalized?

I see, touché: bodiless witness could be "declaration" or "empty definition, output of SILGen to be canonicalized by the differentiation transform".

Adding a [declaration] attribute to solve this parsing ambiguity sounds fine to me.

rxwei · 2019-10-24T05:53:08Z

Alternatively, you can add something like [not_canonicalized] that means the opposite of isDeclaration(), making it a more salient signal that makes you expect the differentiation transform will do stuff on it.

marcrasi

There is already a syntax difference between decl/defn:

sil_differentiability_witness ... {}

vs

sil_differentiability_witness ...

And this is precedented in protocol witness table. You can see the printer that omits the braces and does not print any attribute when it's a declaration: https://github.com/apple/swift/blob/6d178e81ce44b6be44fdd2d9ef7c5ce7e3046709/lib/SIL/SILPrinter.cpp#L2908

So I'll leave the printing and parsing as it is, unless you still have doubts about the current way.

lib/ParseSIL/ParseSIL.cpp

marcrasi · 2019-10-24T20:29:30Z

test/AutoDiff/sil_differentiability_witness.sil

+
+sil @externalFn1 : $@convention(thin) (Float) -> Float
+
+sil @AD__externalFn1__jvp_src_0_wrt_0 : $@convention(thin) (Float) -> (Float, @owned @callee_guaranteed (Float) -> Float) {


Yes it passes. I'll add a test that does that. (Bodiless original with defined jvp/vjp seems like the most common case that will happen in practice so I want to keep a test that does it this way rather than modifying this test to have bodiless jvp/vjp).

dan-zheng · 2019-10-24T21:00:48Z

So I'll leave the printing and parsing as it is, unless you still have doubts about the current way.

Leaving printing and parsing as is (no explicit [declaration] attribute), following the precedent of SIL witness tables, sounds good to me!

We discussed a potential phase-ordering problem where "a differentiability witness with no JVP/VJP could be interpreted as either a declaration or an empty definition", but I believe those two states actually have different syntax:

Declaration: no body.
Empty definition (to be canonicalized): empty body ({}).
Canonical definition: body with JVP/VJP ({ jvp: @..., vjp: @... }).

dan-zheng and others added 2 commits October 23, 2019 14:08

[WIP]

842dad6

Todos: - Update printing/parsing - Consider safer `convertToDefinition` function, from SILWitnessTable. - I think we could defer this and start with straightforward definition/declaration constructors.

parse and serialization for decl-only SILDifferentiabilityWitness

6e3d7e6

marcrasi requested a review from dan-zheng October 23, 2019 23:57

rxwei self-requested a review October 24, 2019 00:06

dan-zheng approved these changes Oct 24, 2019

View reviewed changes

rxwei reviewed Oct 24, 2019

View reviewed changes

include/swift/SIL/SILDifferentiabilityWitness.h Show resolved Hide resolved

rxwei changed the title ~~decl-only SILDifferentiabilityWitness~~ [AutoDiff] decl-only SILDifferentiabilityWitness Oct 24, 2019

rxwei changed the title ~~[AutoDiff] decl-only SILDifferentiabilityWitness~~ [AutoDiff] declaration-only SILDifferentiabilityWitness Oct 24, 2019

rxwei approved these changes Oct 24, 2019

View reviewed changes

add another differentiability witness parsing test

66ca081

marcrasi commented Oct 24, 2019

View reviewed changes

marcrasi merged commit 051e6e0 into swiftlang:tensorflow Oct 24, 2019

marcrasi deleted the sil-diff-witness-definitions branch October 24, 2019 21:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[AutoDiff] declaration-only `SILDifferentiabilityWitness` #27854

[AutoDiff] declaration-only `SILDifferentiabilityWitness` #27854

Uh oh!

marcrasi commented Oct 23, 2019 •

edited by rxwei

Loading

Uh oh!

marcrasi commented Oct 23, 2019

Uh oh!

marcrasi commented Oct 24, 2019

Uh oh!

dan-zheng left a comment

Uh oh!

dan-zheng Oct 24, 2019

Uh oh!

marcrasi Oct 24, 2019

Uh oh!

Uh oh!

Uh oh!

rxwei left a comment

Uh oh!

dan-zheng commented Oct 24, 2019 •

edited

Loading

Uh oh!

rxwei commented Oct 24, 2019

Uh oh!

dan-zheng commented Oct 24, 2019

Uh oh!

rxwei commented Oct 24, 2019 •

edited

Loading

Uh oh!

marcrasi left a comment

Uh oh!

Uh oh!

marcrasi Oct 24, 2019

Uh oh!

dan-zheng commented Oct 24, 2019

Uh oh!

Uh oh!


		sil @externalFn1 : $@convention(thin) (Float) -> Float

		sil @AD__externalFn1__jvp_src_0_wrt_0 : $@convention(thin) (Float) -> (Float, @owned @callee_guaranteed (Float) -> Float) {

[AutoDiff] declaration-only SILDifferentiabilityWitness #27854

[AutoDiff] declaration-only SILDifferentiabilityWitness #27854

Uh oh!

Conversation

marcrasi commented Oct 23, 2019 • edited by rxwei Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcrasi commented Oct 23, 2019

Uh oh!

marcrasi commented Oct 24, 2019

Uh oh!

dan-zheng left a comment

Choose a reason for hiding this comment

Uh oh!

dan-zheng Oct 24, 2019

Choose a reason for hiding this comment

Uh oh!

marcrasi Oct 24, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rxwei left a comment

Choose a reason for hiding this comment

Uh oh!

dan-zheng commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rxwei commented Oct 24, 2019

Uh oh!

dan-zheng commented Oct 24, 2019

Uh oh!

rxwei commented Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcrasi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

marcrasi Oct 24, 2019

Choose a reason for hiding this comment

Uh oh!

dan-zheng commented Oct 24, 2019

Uh oh!

Uh oh!

[AutoDiff] declaration-only `SILDifferentiabilityWitness` #27854

[AutoDiff] declaration-only `SILDifferentiabilityWitness` #27854

marcrasi commented Oct 23, 2019 •

edited by rxwei

Loading

dan-zheng commented Oct 24, 2019 •

edited

Loading

rxwei commented Oct 24, 2019 •

edited

Loading