Skip to content
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

[BridgingHeader] Auto bridging header chaining #78623

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cachemeifyoucan
Copy link
Contributor

@cachemeifyoucan cachemeifyoucan commented Jan 14, 2025

Add ability to automatically chaining the bridging headers discovered from all dependencies module when doing swift caching build. This will eliminate all implicit bridging header imports from the build and make the bridging header importing behavior much more reliable, while keep the compatibility at maximum.

For example, if the current module A depends on module B and C and both B and C are binary modules that uses bridging header, when building module A, dependency scanner will construct a new header that chains three bridging headers together with the option to build a PCH from it. This will make all importing errors more obvious while improving the performance.

@cachemeifyoucan
Copy link
Contributor Author

Please test with following PR;
swiftlang/llvm-project#9818

@swift-ci please smoke test

@cachemeifyoucan cachemeifyoucan changed the title [BrdigingHeader] Auto bridging header chaining [BridgingHeader] Auto bridging header chaining Jan 14, 2025
@cachemeifyoucan cachemeifyoucan force-pushed the eng/bridging-header-auto-chaining branch from c58fd27 to 4e03c48 Compare January 14, 2025 17:41
@cachemeifyoucan
Copy link
Contributor Author

Please test with following PR:
swiftlang/llvm-project#9818

@swift-ci please smoke test

Copy link
Contributor

@artemcm artemcm left a comment

Choose a reason for hiding this comment

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

Formalizing the distinction of .h vs .pch input in the compiler is really nice, thank you.

since the new chained header is written into CAS directly without the need to serialization.

I'm keen to have this be available to all Explicitly-Built Modules builds, for which we will need a way to have the scanner relay the contents of the generated chained header to the driver.

The driver would then have to serialize the chained .h to the filesystem and either use it directly on compile jobs, or feed it to the PCH generation task. We can probably have this be a simple libSwiftScan API that communicates the contents of the header to the driver.

include/swift/ClangImporter/ClangImporter.h Outdated Show resolved Hide resolved
lib/DependencyScan/ModuleDependencyScanner.cpp Outdated Show resolved Hide resolved
lib/DependencyScan/ModuleDependencyScanner.cpp Outdated Show resolved Hide resolved
// consistant ordering.
std::map<std::string, std::string> bridgingHeadersContent;
auto FS = ScanASTContext.SourceMgr.getFileSystem();
for (const auto &moduleID: allSwiftModules) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we run into trouble if the ordering of allSwiftModules is not what we expect? My intuition is that the order in which we plop contents of bridging headers into the chained header matters and that it should be topological. e.g. If A -> B -> C and B and C are binary modules with bridging headers, we should include B's first and then C's.

It looks like resolveSwiftModuleDependencies builds an array that looks like it may already satisfy this, but it's worth making sure this requirement is captured 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.

I don't think allSwiftModules is topo logically sorted yet, just enforcing a deterministic order for now in this PR.

Note the current import ordering is not really following what you suggest as well, since clang importer will including current bridging header/PCH first, then pulling in additional content as swiftmodule is imported.

lib/ClangImporter/ClangModuleDependencyScanner.cpp Outdated Show resolved Hide resolved
@cachemeifyoucan
Copy link
Contributor Author

Formalizing the distinction of .h vs .pch input in the compiler is really nice, thank you.

This actually comes as a side effect that we need to embed the original bridging header path from command-line in swiftmodule, not the generated header that actually used during compilation. I am still working on the SwiftDriver component for that.

I see this is added in the corresponding LLVM PR but not used here. What's the thinking behind AdditionalModules?

I reuse an old PR that intended to unify the actions on the clang side for both getting module deps and TU deps. The original intent was that I can scan for each clang importer during swift compilation as well (where it imports clang modules + bridging header) but that might be an overkill here if we intended for using PCH for bridging header.

@cachemeifyoucan
Copy link
Contributor Author

I'm keen to have this be available to all Explicitly-Built Modules builds, for which we will need a way to have the scanner relay the contents of the generated chained header to the driver.

Yes, that is the plan. Any suggestions for how to generate the header? How to control where the header is written to (maybe -pch-output-dir?) and/or there can be a default location? Note the location needs to consistent otherwise every scan will invalidate all the compilation due to path change.

@artemcm
Copy link
Contributor

artemcm commented Jan 14, 2025

I'm keen to have this be available to all Explicitly-Built Modules builds, for which we will need a way to have the scanner relay the contents of the generated chained header to the driver.

Yes, that is the plan. Any suggestions for how to generate the header? How to control where the header is written to (maybe -pch-output-dir?) and/or there can be a default location? Note the location needs to consistent otherwise every scan will invalidate all the compilation due to path change.

The header should be generated by the driver, with the scanner communicating its contents to the driver, either via libSwiftScan API or JSON.

-pch-output-dir kind of makes sense, and we can encode the header name with something like mainModuleName + scanContextHash + "-ChainedBridging.h". Though it would be good if this worked even without a need for a PCH, in which case this directory flag may be missing. We need some kind of build-temporary location that persists across invocations, which we don't really do outside of incremental builds, today. Hmm.

@cachemeifyoucan
Copy link
Contributor Author

The header should be generated by the driver, with the scanner communicating its contents to the driver, either via libSwiftScan API or JSON.

This is also in the mist of all the dependency scanning tasks. If driver wants to control, it needs to register a callback or just register the path beforehand.

@artemcm
Copy link
Contributor

artemcm commented Jan 14, 2025

The header should be generated by the driver, with the scanner communicating its contents to the driver, either via libSwiftScan API or JSON.

This is also in the mist of all the dependency scanning tasks. If driver wants to control, it needs to register a callback or just register the path beforehand.

If the driver is in charge of placing the header, I don't see a problem with the driver also communicating the path to the scanner, but I am not sure I follow the requirement?

@cachemeifyoucan cachemeifyoucan force-pushed the eng/bridging-header-auto-chaining branch from 4e03c48 to e483ac9 Compare January 17, 2025 23:41
@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan
Copy link
Contributor Author

This is almost there. The only gap I need to close is the embedded header in the swift binary module is the chained header, rather than the original header. This is only not ok in very corner cases which current build also doesn't work well and need to pass extra information from scanner to swift-frontend.

There is no need for a new scanner API and it should work for non caching build too.

@cachemeifyoucan cachemeifyoucan force-pushed the eng/bridging-header-auto-chaining branch from e483ac9 to 67e03df Compare January 21, 2025 21:53
@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan cachemeifyoucan force-pushed the eng/bridging-header-auto-chaining branch 2 times, most recently from 17b692c to b769acf Compare January 21, 2025 23:40
@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

include/swift/AST/ModuleDependencies.h Outdated Show resolved Hide resolved
include/swift/DependencyScan/ModuleDependencyScanner.h Outdated Show resolved Hide resolved
include/swift/Frontend/FrontendOptions.h Outdated Show resolved Hide resolved
@@ -369,6 +375,9 @@ class FrontendOptions {
/// Emit remarks indicating use of the serialized module dependency scanning cache.
bool EmitDependencyScannerCacheRemarks = false;

/// The patch at which the dependency scanner can write generated files.
std::string ScannerOutputDir;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I don't think the scanner should be writing out any generated files. Rather, it should communicate any files that need generating to the client, i.e. SwiftDriver, and then have the client do the required IO.

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that for CAS purposes we do need to have the scanner have the information of where the chained bridging header will ultimately be serialized to, we could add an option for this specifically: -chained-bridging-header-output-path that the build system client can provide.

lib/AST/ModuleDependencies.cpp Outdated Show resolved Hide resolved
@cachemeifyoucan cachemeifyoucan force-pushed the eng/bridging-header-auto-chaining branch from b769acf to 5a7e139 Compare January 21, 2025 23:50
@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan cachemeifyoucan force-pushed the eng/bridging-header-auto-chaining branch 2 times, most recently from c0812fb to 6e2dbfb Compare January 22, 2025 19:54
@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

1 similar comment
@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan cachemeifyoucan force-pushed the eng/bridging-header-auto-chaining branch from 6e2dbfb to 055cccd Compare January 22, 2025 21:55
@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan cachemeifyoucan force-pushed the eng/bridging-header-auto-chaining branch from 055cccd to 28fd1af Compare January 24, 2025 17:52
@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan cachemeifyoucan force-pushed the eng/bridging-header-auto-chaining branch from 28fd1af to 57c20e4 Compare January 25, 2025 00:05
@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan cachemeifyoucan force-pushed the eng/bridging-header-auto-chaining branch from 57c20e4 to d90ff24 Compare January 27, 2025 03:24
@cachemeifyoucan
Copy link
Contributor Author

Please test with following PR:
swiftlang/llvm-project#9818

@swift-ci please smoke test

@cachemeifyoucan cachemeifyoucan force-pushed the eng/bridging-header-auto-chaining branch from d90ff24 to 1527762 Compare January 27, 2025 17:12
@cachemeifyoucan
Copy link
Contributor Author

Please test with following PR:
swiftlang/llvm-project#9818

@swift-ci please smoke test

@cachemeifyoucan cachemeifyoucan force-pushed the eng/bridging-header-auto-chaining branch from 1527762 to e4a3e4c Compare January 27, 2025 18:12
@cachemeifyoucan
Copy link
Contributor Author

Please test with following PR:
swiftlang/llvm-project#9818

@swift-ci please smoke test

@cachemeifyoucan
Copy link
Contributor Author

@artemcm This is ready to be reviewed.

@cachemeifyoucan cachemeifyoucan force-pushed the eng/bridging-header-auto-chaining branch from e4a3e4c to bd41955 Compare January 28, 2025 00:44
@cachemeifyoucan
Copy link
Contributor Author

Please test with following PR:
swiftlang/llvm-project#9818

@swift-ci please smoke test

Add ability to automatically chaining the bridging headers discovered from all
dependencies module when doing swift caching build. This will eliminate all
implicit bridging header imports from the build and make the bridging header
importing behavior much more reliable, while keep the compatibility at maximum.

For example, if the current module A depends on module B and C, and both B and
C are binary modules that uses bridging header, when building module A,
dependency scanner will construct a new header that chains three bridging
headers together with the option to build a PCH from it. This will make all
importing errors more obvious while improving the performance.
@cachemeifyoucan cachemeifyoucan force-pushed the eng/bridging-header-auto-chaining branch from bd41955 to 8b2df4f Compare January 28, 2025 23:01
@cachemeifyoucan
Copy link
Contributor Author

Please test with following PR:
swiftlang/llvm-project#9818

@swift-ci please smoke test

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.

2 participants