Skip to content

[clang][modules] Only compute affecting module maps with implicit search #87849

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

Merged
merged 1 commit into from
Apr 10, 2024
Merged

[clang][modules] Only compute affecting module maps with implicit search #87849

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions clang/lib/Serialization/ASTWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,13 @@ static TypeCode getTypeCodeForTypeClass(Type::TypeClass id) {

namespace {

std::set<const FileEntry *> GetAffectingModuleMaps(const Preprocessor &PP,
Module *RootModule) {
std::optional<std::set<const FileEntry *>>
GetAffectingModuleMaps(const Preprocessor &PP, Module *RootModule) {
// Without implicit module map search, there's no good reason to know about
// any module maps that are not affecting.
if (!PP.getHeaderSearchInfo().getHeaderSearchOpts().ImplicitModuleMaps)
return std::nullopt;
Comment on lines +166 to +169
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this is correct -- we can know about non-affecting module map files because they were specified on the command line using -fmodule-map-file= and either weren't actually used or (as happens in our case) we got the same information from a PCM file and so didn't need the module map file.

I think this check is a regression; can it be removed?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hm, I see that adding this check was the whole point of the patch. I don't think this works -- it's not feasible for the build system to know whether a module map file would affect a compilation or not.

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 wasn't aware that you're passing unused module map files to the compiler. In that case I can introduce a separate flag to control this functionality and only disable the computation of affecting module maps for our flavor of explicit modules where this is a no-op. Sounds good?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, that sounds great, thank you. In our case, it's not that the module map files are unused, it's that we specify the same -fmodule-map-file= flag to all of our compilations, and so we usually get the information from a .pcm file rather than from the module map file, and it turns out that pruning out the repeated module maps make a rather large difference to our source location address space usage :)

All that said, we have some other ideas for how to address the problems we're seeing with source location address space usage. Maybe we can get to a point where we're not relying on this at all -- I think this patch is probably the right behavior, even though I think it's contributing to a regression for us, so I think this new behavior should be the default, and we can clean up the flag once we don't need it any more.


SmallVector<const Module *> ModulesToProcess{RootModule};

const HeaderSearch &HS = PP.getHeaderSearchInfo();
Expand Down Expand Up @@ -4733,8 +4738,16 @@ void ASTWriter::computeNonAffectingInputFiles() {
if (!Cache->OrigEntry)
continue;

if (!isModuleMap(File.getFileCharacteristic()) ||
llvm::is_contained(AffectingModuleMaps, *Cache->OrigEntry))
// Don't prune anything other than module maps.
if (!isModuleMap(File.getFileCharacteristic()))
continue;

// Don't prune module maps if all are guaranteed to be affecting.
if (!AffectingModuleMaps)
continue;

// Don't prune module maps that are affecting.
if (llvm::is_contained(*AffectingModuleMaps, *Cache->OrigEntry))
continue;

IsSLocAffecting[I] = false;
Expand Down