Skip to content

[Caching] Create clang importer from cc1 args directly #71118

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Mar 11, 2024
Merged

[Caching] Create clang importer from cc1 args directly #71118

merged 4 commits into from
Mar 11, 2024

Conversation

cachemeifyoucan
Copy link
Contributor

When caching build is enabled, teach dependency scanner to report command-lines with -direct-clang-cc1-module-build so the later compilation can instantiate clang importer with cc1 args directly. This avoids running clang driver code, which might involve file system lookups, which are the file deps that are not captured and might result in different compilation mode.

rdar://119275464

@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan
Copy link
Contributor Author

Currently, only enable the new code path when caching is enabled since it is quite a big behavior change. Caching build requires the change since its restricted file system will definitely alter the behavior for clang importer. For normal explicit module build, this will still help with less lookup during the actual compilation, but we can enable that later.

@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

1 similar comment
@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

This was referenced Feb 16, 2024
Closed
Merged
@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan
Copy link
Contributor Author

Rebase to resolve conflict. After some extensive testing, I feel pretty confident to turn this on for caching only. Ready to be reviewed.

@artemcm @DougGregor @benlangmuir @beccadax Since this is a pretty big behavior change, do you have any concern turn on this for all explicit module build? Will run a separate qualification for that.

@artemcm
Copy link
Contributor

artemcm commented Feb 22, 2024

Rebase to resolve conflict. After some extensive testing, I feel pretty confident to turn this on for caching only. Ready to be reviewed.

@artemcm @DougGregor @benlangmuir @beccadax Since this is a pretty big behavior change, do you have any concern turn on this for all explicit module build? Will run a separate qualification for that.

Given sufficient qualification I don't have a concern with enabling this for all Explicit Module Builds. But it would be good to have that enablement be a separate change after this one that we hold off merging until we're able to get enough confidence/validation, or land behind a flag temporarily which we can use to do testing with.

@benlangmuir
Copy link
Contributor

Since this is a pretty big behavior change, do you have any concern turn on this for all explicit module build? Will run a separate qualification for that.

Doing this for all explicit builds seems best to me. Not only this reduces the differences in behaviour for caching, but it also means that clang modules used by swift will behave more like clang modules used by clang -- clang's dependency scanner has been using -cc1 for explicit module builds for a while now.

But it would be good to have that enablement be a separate change after this one that we hold off merging until we're able to get enough confidence/validation, or land behind a flag temporarily which we can use to do testing with.

Flag seems like a good idea to me to make it easier to control until we're ready to flip the default for non-caching.

@cachemeifyoucan cachemeifyoucan mentioned this pull request Feb 23, 2024
Merged
@cachemeifyoucan
Copy link
Contributor Author

Add a flag to turn on this new behavior for non-caching build. Add some nice to have swift-driver support here: swiftlang/swift-driver#1550

@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test linux platform

@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan
Copy link
Contributor Author

Rebase to resolve merge conflict. Still waiting for review :)

benlangmuir
benlangmuir reviewed Mar 1, 2024
View reviewed changes
lib/ClangImporter/ClangImporter.cpp
HSOpts.ImplicitModuleMaps = 0;
HSOpts.VFSOverlayFiles.clear();
HSOpts.UserEntries.clear();
HSOpts.SystemHeaderPrefixes.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we switch this to use tooling::dependencies::configureInvocationForCaching by passing in the include-tree root ID to this function (optionally, if caching is enabled)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is the eventual goal. But note following for current setup. This is creating a ClangImporter that is not using include-tree even when caching is enabled, but a regular translation unit that imports modules that are built with include tree. It needs a different set of options from an include tree build, thus it has a customized option clearing function and may need some other tweaking.
In the future, we should run a dependency scanning of the clang importer instance then let clang scanner return the arguments.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we can't move completely to configureInvocationForCaching, would it make sense to take a similar approach to that function for how to handle HeaderSearchOptions? In clang we clear the whole HSOpts, and only preserve a few specific settings. That makes it much easier to know what is intentionally being kept.

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 think that makes sense if we have depscanning. Right now, it is matching the behavior in
ClangImporterOptions::getReducedExtraArgsForSwiftModuleDependency, just in a more elegant way.

@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan
Copy link
Contributor Author

ping~

benlangmuir
benlangmuir reviewed Mar 7, 2024
View reviewed changes
lib/ClangImporter/ClangImporter.cpp
HSOpts.ImplicitModuleMaps = 0;
HSOpts.VFSOverlayFiles.clear();
HSOpts.UserEntries.clear();
HSOpts.SystemHeaderPrefixes.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

If we can't move completely to configureInvocationForCaching, would it make sense to take a similar approach to that function for how to handle HeaderSearchOptions? In clang we clear the whole HSOpts, and only preserve a few specific settings. That makes it much easier to know what is intentionally being kept.

benlangmuir
benlangmuir approved these changes Mar 7, 2024
View reviewed changes
Copy link
Contributor

@benlangmuir benlangmuir left a comment

Choose a reason for hiding this comment

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

LGTM, but probably deserves review from @artemcm as well

@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan
[Caching] Create clang importer from cc1 args directly
cdeef58 
When caching build is enabled, teach dependency scanner to report
command-lines with `-direct-clang-cc1-module-build` so the later
compilation can instantiate clang importer with cc1 args directly. This
avoids running clang driver code, which might involve file system
lookups, which are the file deps that are not captured and might result
in different compilation mode.

rdar://119275464
@cachemeifyoucan
[swiftinterface] Improve target overwrite for the swiftinterface
9f73681 
In certain cases (e.g. using arm64e interface to build arm64 target),
the target needs to be updated when building swiftinterface. Push the
target overwrite as early as possible to swiftinterface parsing by
providing a preferred target to relevant functions. In such cases, the
wrong target is never observed by other functions to avoid errors like
the sub-invocation was partially setup for the wrong target.
@cachemeifyoucan
[Caching] Support index while building when caching on
fb3c268 
Model indexing output as an optional output from the swift compiler
as the build system has no knowledge about them and they can be
regenerated by indexer. Make sure the indexing store output is produced
when cache hit so the compilation is done for the module. If cache hit,
no indexing data is produced since no compilation is done.

rdar://123331335
@cachemeifyoucan
[ScanDependency] Add -experimental-clang-importer-direct-cc1-scan
cb17ea8 
Add an experimental option to tell dependency scanner to report clang
cc1 args should be used to construct clang importer in all constructed
swift-frontend tasks.
@cachemeifyoucan
Copy link
Contributor Author

@swift-ci please smoke test

@cachemeifyoucan
Copy link
Contributor Author

@artemcm Ping. Most of the PR should have no changes to non-caching build unless the new option is explicated used. The only behavior changing part is separated out as 9f73681. Let me know if this is good to merge as I have lots of other patches that can cause conflict with this one.

artemcm
artemcm approved these changes Mar 11, 2024
View reviewed changes
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.

This looks good. Thank you @cachemeifyoucan

@cachemeifyoucan cachemeifyoucan merged commit 177bbe6 into swiftlang:main Mar 11, 2024
@drodriguez
Copy link
Contributor

I am trying to find the reason for an internal failure in a Linux system. For it to work, we need to provide Clang with --gcc-toolchain=/foo/bar to point to the GCC toolchain in a non-standard location, so we normally use -Xcc --gcc-toolchain=/foo/bar. It seems that this PR might forward that parameter (which is intended for the Clang driver) to cc1, failing many of the tests with unknown argument: '--gcc-toolchain=/foo/bar' … error: clang importer creation failed.

Is there a way of still using the Clang driver or is that completely gone? Is there a good place to filter then non-cc1 arguments somehow? I am not sure that removing --gcc-toolchain from the command lines will work at all, since it expands to many search paths and options (and I can imagine some other similar options that the Clang driver expands to many options in cc1).

@cachemeifyoucan
Copy link
Contributor Author

I am trying to find the reason for an internal failure in a Linux system. For it to work, we need to provide Clang with --gcc-toolchain=/foo/bar to point to the GCC toolchain in a non-standard location, so we normally use -Xcc --gcc-toolchain=/foo/bar. It seems that this PR might forward that parameter (which is intended for the Clang driver) to cc1, failing many of the tests with unknown argument: '--gcc-toolchain=/foo/bar' … error: clang importer creation failed.

Is there a way of still using the Clang driver or is that completely gone? Is there a good place to filter then non-cc1 arguments somehow? I am not sure that removing --gcc-toolchain from the command lines will work at all, since it expands to many search paths and options (and I can imagine some other similar options that the Clang driver expands to many options in cc1).

The new option is supposed to be only used for explicit module build, which requires dependency scanning. The -Xcc command passed to the swift-driver (or swiftc) is still clang driver flag and that has not changed. There has always been a special mode (mostly for building PCM) that changes the meaning of -Xcc flag to swift-frontend to be clang cc1 flag, from which you have to provide all the arguments to initialize clang importer. This patch just add a new option to ask swift dependency scanner to return options to initialize all swift-frontend directly with cc1 options.

There is a follow-up PR in swift-driver to tell it not to forward -Xcc option from the command-line to swift-frontend anymore. swiftlang/swift-driver#1550. It doesn't need to be forwarded because swift dependency scanner has already seen the driver flag and created the list of cc1 flag that is corresponding driver options and swift-frontend does not need to run clang driver again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benlangmuir benlangmuir benlangmuir approved these changes

@artemcm artemcm artemcm approved these changes

@akyrtzi akyrtzi Awaiting requested review from akyrtzi

@jansvoboda11 jansvoboda11 Awaiting requested review from jansvoboda11

@tshortli tshortli Awaiting requested review from tshortli tshortli is a code owner

@zoecarver zoecarver Awaiting requested review from zoecarver zoecarver is a code owner

@hyp hyp Awaiting requested review from hyp

@egorzhdan egorzhdan Awaiting requested review from egorzhdan egorzhdan is a code owner

@DougGregor DougGregor Awaiting requested review from DougGregor DougGregor is a code owner

@beccadax beccadax Awaiting requested review from beccadax beccadax is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cachemeifyoucan @artemcm @benlangmuir @drodriguez