Skip to content

[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

Merged
merged 4 commits into from
Apr 8, 2020

Conversation

dan-zheng
Copy link
Contributor

@dan-zheng dan-zheng commented Apr 7, 2020

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 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.

Unblocks end-to-end differentiation transform testing.

`SourceFile::addVisibleDecl` is an unnecessary API.
It was upstreamed in swiftlang#30821.
@dan-zheng
Copy link
Contributor Author

@swift-ci Please test

Copy link
Contributor Author

@dan-zheng dan-zheng left a 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
Copy link
Contributor Author

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())) {}
Copy link
Contributor Author

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.

Comment on lines +2714 to +2722
for (auto *decl : TopLevelDecls) {
if (decl->getFullName().matchesRef(name))
result.push_back(decl);
}
Copy link
Contributor Author

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.

@swift-ci
Copy link
Contributor

swift-ci commented Apr 7, 2020

Build failed
Swift Test Linux Platform
Git Sha - 61dcb0c49a3415b23dc40d36ea3288ac76689c33

@swift-ci
Copy link
Contributor

swift-ci commented Apr 8, 2020

Build failed
Swift Test OS X Platform
Git Sha - 61dcb0c49a3415b23dc40d36ea3288ac76689c33

`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.
@dan-zheng dan-zheng force-pushed the synthesized-file-unit branch from 61dcb0c to f7a9eed Compare April 8, 2020 01:31
@dan-zheng
Copy link
Contributor Author

@swift-ci Please test

@dan-zheng dan-zheng requested review from marcrasi and rxwei April 8, 2020 01:33
@swift-ci
Copy link
Contributor

swift-ci commented Apr 8, 2020

Build failed
Swift Test Linux Platform
Git Sha - 61dcb0c49a3415b23dc40d36ea3288ac76689c33

@swift-ci
Copy link
Contributor

swift-ci commented Apr 8, 2020

Build failed
Swift Test OS X Platform
Git Sha - 61dcb0c49a3415b23dc40d36ea3288ac76689c33

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.
@dan-zheng
Copy link
Contributor Author

@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented Apr 8, 2020

Build failed
Swift Test Linux Platform
Git Sha - f7a9eed

@swift-ci
Copy link
Contributor

swift-ci commented Apr 8, 2020

Build failed
Swift Test OS X Platform
Git Sha - f7a9eed

Copy link
Contributor

@rxwei rxwei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice

@dan-zheng
Copy link
Contributor Author

Merging to unblock progress. Happy to address any feedback later!

SynthesizedFileUnitis created only as needed by the differentiation transform (de4deb5), other compiler invocations should be unaffected.

@dan-zheng dan-zheng merged commit f27f1cd into swiftlang:master Apr 8, 2020
dan-zheng added a commit to swiftlang/llvm-project that referenced this pull request Apr 8, 2020
@dan-zheng dan-zheng deleted the synthesized-file-unit branch April 8, 2020 06:48
JDevlieghere pushed a commit to swiftlang/llvm-project that referenced this pull request Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants