-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[AutoDiff] Add generated implicit declarations to SynthesizedFileUnit. #30863
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
Conversation
`SourceFile::addVisibleDecl` is an unnecessary API. It was upstreamed in swiftlang#30821.
@swift-ci Please test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@slavapestov: I started this change based on your feedback: #30845 (comment).
Could you please review? I left comments about potential changes to SynthesizedFileUnit
.
// name and a special string. | ||
llvm::MD5 hash; | ||
hash.update(getParentModule()->getName().str()); | ||
// TODO: Use a more robust discriminator for synthesized files. Pick something |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: consider robust discriminator ideas for synthesized files?
SynthesizedFileUnit
does not store any contents (e.g. name) useful for discrimination. Perhaps we could use file index (the nth synthesized file in the module), though I'm not sure that file order is robust.
ADContext::ADContext(SILModuleTransform &transform) | ||
: transform(transform), module(*transform.getModule()), | ||
passManager(*transform.getPassManager()) {} | ||
passManager(*transform.getPassManager()), | ||
synthesizedFile(*createNewSynthesizedFileUnit(module.getSwiftModule())) {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, ADContext
in the differentiation transform is responsible for creating a SynthesizedFileUnit
.
Instead, we could add utilities for creating a SynthesizedFileUnit
on ModuleDecl
:
Optional<SynthesizedFileUnit *> ModuleDecl::getSynthesizedFileUnit
SynthesizedFileUnit *ModuleDecl::getOrCreateSynthesizedFileUnit
SynthesizedFileUnit
is a new construct, so we could establish invariants like "each module only has one synthesized file unit". I don't know about benefits of these invariants if SynthesizedFileUnit
is used only by the differentiation transform.
for (auto *decl : TopLevelDecls) { | ||
if (decl->getFullName().matchesRef(name)) | ||
result.push_back(decl); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: SourceFile
and ModuleDecl
use SourceLookupCache
to store declarations, which enables more efficient lookup. But I didn't use SourceLookupCache
for SynthesizedFileUnit
because that involves publicly exposing a mutating SourceLookupCache::addTopLevelDecl
function.
TopLevelDecls
is a SmallVector
instead of a DenseMap
, copying the old DerivedFileUnit
. DenseMap
makes lookup more efficient, but I think it makes SynthesizedFileUnit::getTopLevelDecls
less efficient.
Build failed |
Build failed |
`SynthesizedFileUnit` is a container for synthesized declarations. Currently, it only supports module-level declarations. It is used by the SIL differentiation transform, which generates implicit struct and enum declarations.
Add implicit declarations generated by the differentiation transform to a `SynthesizedFileUnit` instead of an ad-hoc pre-existing `SourceFile`. Resolves TF-1232: type reconstruction for AutoDiff-generated declarations. Previously, type reconstruction failed because retroactively adding declarations to a `SourceFile` did not update name lookup caches.
61dcb0c
to
f7a9eed
Compare
@swift-ci Please test |
Build failed |
Build failed |
Make `ADContext` lazily create a `SynthesizedFileUnit` instead of creating one during `ADContext` construction. This avoids always creating a `SynthesizedFileUnit` in every module, since differentiation is a mandatory transform that always runs. It was nonetheless useful to test always creating a `SynthesizedFileUnit` for testing purposes.
@swift-ci Please test |
Build failed |
Build failed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice
Merging to unblock progress. Happy to address any feedback later!
|
Friend PR: swiftlang/swift#30863 (cherry picked from commit 33154ab)
Add
SynthesizedFileUnit
: a container for synthesized declarations. Currently, it only supports module-level declarations.Use
SynthesizedFileUnit
in the differentiation transform: add implicit generated declarations generated to aSynthesizedFileUnit
instead of an ad-hoc pre-existingSourceFile
.Resolves TF-1232: type reconstruction for AutoDiff-generated declarations.
Previously, type reconstruction failed because retroactively adding declarations to a
SourceFile
did not update name lookup caches.Unblocks end-to-end differentiation transform testing.