Skip to content

[Frontend] Emit incremental compilation info from some emit-module jobs #38211

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 2 commits into from
Aug 14, 2021

Conversation

xymus
Copy link
Contributor

@xymus xymus commented Jul 1, 2021

Emit the incremental fine-grained information from the emit-module job to improve the emit-module-separately build mode compatibility with incremental compilation. Note that emit-module doesn't usually parses or type-checks non-inlinable function bodies so it may be unaware of some dependencies.

@xymus
Copy link
Contributor Author

xymus commented Jul 1, 2021

@swift-ci Please smoke test

@xymus
Copy link
Contributor Author

xymus commented Jul 1, 2021

I'm mostly putting this PR up in support for testing swiftlang/swift-driver#730.

@CodaFi Are there tests in the compiler repo that could be extended to test this or are they on the driver side?

@xymus xymus requested review from CodaFi and davidungar July 1, 2021 22:58
@CodaFi
Copy link
Contributor

CodaFi commented Jul 1, 2021

There are some tests under test/Incremental that could be extended for frontend-specific testing. We try to keep the integration tests in the driver these days though, yeah.

@xymus
Copy link
Contributor Author

xymus commented Jul 1, 2021

swiftlang/swift-driver#730
@swift-ci Please test

@swift-ci
Copy link
Contributor

swift-ci commented Jul 1, 2021

Build failed
Swift Test Linux Platform
Git Sha - 63b283c5281f085296d470ee616094002bb87eef

@swift-ci
Copy link
Contributor

swift-ci commented Jul 2, 2021

Build failed
Swift Test OS X Platform
Git Sha - 63b283c5281f085296d470ee616094002bb87eef

Copy link
Contributor

@davidungar davidungar left a comment

Choose a reason for hiding this comment

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

Beautiful!

@xymus
Copy link
Contributor Author

xymus commented Jul 2, 2021

@swift-ci Please smoke test

@xymus xymus force-pushed the incr-info-and-emit-module branch from 63b283c to 7ec8c4e Compare August 6, 2021 17:19
@xymus
Copy link
Contributor Author

xymus commented Aug 6, 2021

@swift-ci Please smoke test

auto *Mod = MSF.get<ModuleDecl *>();

StringRef fileFingerprint;
if (auto Mod = MSF.dyn_cast<ModuleDecl *>()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused as to how we see SourceFiles here. There shouldn't be any primaries, so this path should only be fed by ModuleDecls, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Emit module only has source file inputs that end up here. Do you think that shouldn't happen?

I wouldn't expect this job to be passed the primary flag but I could double check in the tests where this failed.

Copy link
Contributor

Choose a reason for hiding this comment

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

My expectation is that separate module mode behaves like WMO, not like single-file mode. There shouldn't be primaries here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! IIUC a jobs with the action -emit-module and using a primary-file would be used to create a partial swiftmodule file, so I'll modify this code to avoid jobs with a SourceFile and it should just generate the graph for module-wide emit-module jobs.

@@ -343,7 +343,8 @@ void CompilerInstance::setupDependencyTrackerIfNeeded() {
// If we have an output path specified, but no other tracking options,
// default to non-system dependency tracking.
if (opts.InputsAndOutputs.hasDependencyTrackerPath() ||
!opts.IndexStorePath.empty()) {
!opts.IndexStorePath.empty() ||
opts.RequestedAction == FrontendOptions::ActionType::EmitModuleOnly) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CodaFi What do you think of forcing dependency tracking for all emit module only jobs? The data structure configured here is used later to generate the fine grained dependency graph which is also now built by default with all emit module only jobs.

Copy link
Contributor

Choose a reason for hiding this comment

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

This part of the code is about forcing system (read: SDK) dependencies on and off when we import stuff via the clang importer. I don't think you need to amend this logic. The user should be the one in control here with -track-system-dependencies.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is no more necessary when fixing the SourceFile issue above.

@xymus xymus force-pushed the incr-info-and-emit-module branch from 7ec8c4e to fd2bea0 Compare August 6, 2021 22:34
@xymus
Copy link
Contributor Author

xymus commented Aug 6, 2021

@swift-ci Please smoke test

@xymus
Copy link
Contributor Author

xymus commented Aug 9, 2021

swiftlang/swift-driver#730
@swift-ci Please smoke test

@xymus xymus force-pushed the incr-info-and-emit-module branch from fd2bea0 to 26c44ef Compare August 11, 2021 23:35
@xymus
Copy link
Contributor Author

xymus commented Aug 11, 2021

@swift-ci Please smoke test

xymus added 2 commits August 13, 2021 16:53
Enable emitting the module-level incremental fine-grained compilation
information from the emit-module job for incremental compilation to
work with emit-module-separately.
The differentiation and actor logics insert SynthesizedFileUnit in
SourceFile modules. Accepting these file units in populateMemberCache
allow to cache all the top-level decls of source file modules.
@xymus xymus force-pushed the incr-info-and-emit-module branch from 26c44ef to 6fd9102 Compare August 13, 2021 23:54
@xymus
Copy link
Contributor Author

xymus commented Aug 13, 2021

@swift-ci Please smoke test

@xymus xymus changed the title [Frontend] Emit incremental compilation info from emit-module jobs [Frontend] Emit incremental compilation info from some emit-module jobs Aug 13, 2021
@xymus
Copy link
Contributor Author

xymus commented Aug 14, 2021

@swift-ci Please smoke test Linux

@xymus
Copy link
Contributor Author

xymus commented Aug 14, 2021

@swift-ci Please smoke test Windows

@xymus xymus merged commit 392ba00 into swiftlang:main Aug 14, 2021
@xymus xymus deleted the incr-info-and-emit-module branch August 14, 2021 05:10
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.

5 participants