-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[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 |
dan-zheng
left a comment
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.
lib/AST/Module.cpp
Outdated
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.
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::getSynthesizedFileUnitSynthesizedFileUnit *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.
lib/AST/Module.cpp
Outdated
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 |
rxwei
left a comment
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
SynthesizedFileUnitin the differentiation transform: add implicit generated declarations generated to aSynthesizedFileUnitinstead 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
SourceFiledid not update name lookup caches.Unblocks end-to-end differentiation transform testing.