[AutoDiff] DifferentiabilityWitnessFunctionInst stores pointer to witness #27919
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dan-zheng
approved these changes
Oct 29, 2019
|
@swift-ci please test tensorflow |
rxwei
approved these changes
Oct 29, 2019
lib/ParseSIL/ParseSIL.cpp
Outdated
| // Check whether original function generic signature and parsed witness | ||
| // generic have the same generic parameters. | ||
| auto areGenericParametersConsistent = [&]() { | ||
| llvm::DenseSet<GenericParamKey> genericParamKeys; |
| dwfi->getParameterIndices()->getNumIndices(), | ||
| dwfi->getResultIndices()->getNumIndices(), | ||
| parameterAndResultIndices); | ||
| SILOneOperandLayout::emitRecord( |
lib/ParseSIL/ParseSIL.cpp
Outdated
| auto *resultIndexSet = IndexSubset::get( | ||
| P.Context, origFnType->getNumResults(), resultIndices); | ||
| AutoDiffConfig config(parameterIndexSet, resultIndexSet, witnessGenSig); | ||
| return {{config, originalFunction}}; |
There was a problem hiding this comment.
Double curly braces are a bit cryptic. How about:
Suggested change
| return {{config, originalFunction}}; | |
| return std::make_pair(config, originalFunction); |
|
Please feel free to merge it now and address my minor comments later. I don't want to block progress getting retrodiff landed! |
|
@swift-ci please test tensorflow |
1 similar comment
|
@swift-ci please test tensorflow |
|
@swift-ci please test tensorflow macos |
dan-zheng
added a commit
to dan-zheng/swift
that referenced
this pull request
Oct 30, 2019
Remove unused variable `Attr3`. It is no longer necessary after swiftlang#27919.
dan-zheng
added a commit
that referenced
this pull request
Oct 30, 2019
Remove unused variable `Attr3`. It is no longer necessary after #27919.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We want the
DifferentiabilityWitnessFunctionInstto store a pointer to the witness because this makes the instruction data structure smaller and simpler, and makes misuse (e.g. referencing a witness that's not declared in the SIL module) harder.This PR mostly preserves the existing SIL syntax & rules. The changes are:
sil_differentiability_witnessdeclarations are now required to come beforedifferentiability_witness_functioninstructions that reference them, so that we don't have to add "forward declaration" logic for them.sil_differentiability_witnessconstraints now look like<τ_0_0 where τ_0_0 : _Differentiable>instead of[where T : _Differentiable]. This allows me to share parsing logic between thesil_differentiability_witnessconstraints and thedifferentiability_witness_function, which simplifies the code and also avoids a problem [1].This PR changes the
DifferentiabilityWitnessFunctionInstserialization format to store the witness mangled name rather than the original function name plus indices and generic constraints.Detailed outline of changes:
DifferentiabilityWitnessFunctionInstnow contains aSILDifferentiabilityWitness*instead of a pointer to original function, indices, and generic signature. (credit to @dan-zheng, I just copied these changes from his commit)SILModulefor looking upSILDifferentiabilityWitness*because parsing and deserialization need to find the rightSILDifferentiabilityWitness*to put in the inst.sil_differentiability_witnessdeclarations before function declarations<T where T ...>instead of[where T ...]insil_differentiability_witnessdeclarations.parseSILDifferentiabilityWitnessConfigAndFunction, and use this for parsing bothsil_differentiability_witnessanddifferentiability_witness_function. This simplifies the code and avoids the problem [1].differentiability_witness_function, look for theSILDifferentiabilityWitness*in the module.sil_differentiability_witnessdeclarations that come at the beginning of the file.SILInstDifferentiabilityWitnessFunctionLayoutfrom the serialization format, because theDifferentiabilityWitnessFunctionInstnow fits into theSILOneOperandLayout.DifferentiabilityWitnessFunctionInstaSILOneOperandLayoutwith the mangled witness name.DifferentiabilityWitnessFunctionInstby reading the name and deserializing the corresponding witness.[1] The avoided problem: The previous code for parsing
sil_differentiability_witnessconstraints of the form[where T : _Differentiable]fails to handle the case where the original function is declaration-only, because the code assumes that there is a generic environment, and declaration-only functions do not have a generic environment. Rather than fixing this code, it seemed better to delete it and use the existing code that parsesdifferentiability_witness_functionconstraints, which does not suffer from this problem.