[AutoDiff] remove some jvp:, vjp: from stdlib #28762
Conversation
There was a problem hiding this comment.
Nice!
If rewriting @differentiable(jvp:vjp:) to @derivative in the stdlib required compiler changes, is it possible to add unit tests for the compiler changes to ensure coverage beyond stdlib compilation?
I wonder what issues prevented rewriting Edit: I should've read the PR description more carefully, sorry - I see it mentions "incorrect pullback type" diagnostics.@differentiable(jvp:vjp:) without the compiler changes.
| auto vectorTy = valueResultConf.getTypeWitnessByName( | ||
| valueResultType, Ctx.Id_TangentVector); | ||
|
|
||
| // Compute the actual differential/pullback type that we use for comparison |
There was a problem hiding this comment.
Note: #28738 upstreams @derivative type-checking to master. I can handle cherry-picking this fix to master after that PR is merged.
I wonder if it's possible to write a unit test that can be upstreamed with the fix?
Food for thought: after code is upstreamed, perhaps we should make changes on master and merge them (or let them be merged) to tensorflow. This saves some effort compared with committing to both tensorflow and master, since code must eventually land in master.
| auto *ptr = llvm::DenseMapInfo<void *>::getEmptyKey(); | ||
| return {static_cast<IndexSubset *>(ptr), static_cast<IndexSubset *>(ptr), | ||
| DenseMapInfo<GenericSignature>::getEmptyKey()}; | ||
| nullptr}; |
There was a problem hiding this comment.
For future reference: I wonder what errors arise from using DenseMapInfo<GenericSignature>::getEmptyKey() here?
I believe we should, for libraries that are built for distribution. |
| } | ||
|
|
||
| extension Tracked where T : Differentiable, T == T.TangentVector { | ||
| @usableFromInline |
There was a problem hiding this comment.
Note for later: We should make @derivative and @transpose imply @usableFromInline for internal declarations.
|
Some patches depend on this PR, so let's try to merge it now. |
|
@swift-ci Please test tensorflow |
|
Good idea about the unit tests. I'll write some and send a followup PR, so that I don't block merging this. |
Sounds good to me. Therefore, I will keep the |
Upstream `@derivative` attribute type-checking fixes regarding derivative generic signatures with all concrete generic parameters. Cherry-picked from: - swiftlang#28762 - swiftlang#28772
Removes some
jvp:andvjp:from the stdlib. This is partial progress towards unblocking TF-1001.Some bugfixes were necessary:
DenseMapInfo<AutoDiffConfig>wasn't canonicalizing theGenericSignature, causing equivalent configs to appear distinct.@derivative(of:)typechecking was not taking the generic signature into account when comparing actual/expected differential/pullback types, causing incorrect "incorrect pullback type" diagnositcs.MF.getIdentifierdoesn't work on special identifiers likeinit, so@derivative(of: init)didn't work.Open style question: When we have a choice about whether to put a
@differentiableattribute on the original declaration, should we? This PR leaves all the@differentiableattributes, but it should also work if this PR removes them.