Skip to content

[C++20] [Modules] Don't generate call to an imported module that dont init anything #67638

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
Sep 28, 2023
Merged

[C++20] [Modules] Don't generate call to an imported module that dont init anything #67638

Show file tree
Hide file tree
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
6 changes: 6 additions & 0 deletions clang/include/clang/Basic/Module.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,10 @@ class alignas(8) Module {
/// to a regular (public) module map.
unsigned ModuleMapIsPrivate : 1;

/// Whether this C++20 named modules doesn't need an initializer.
/// This is only meaningful for C++20 modules.
unsigned NamedModuleHasNoInit : 1;

/// Describes the visibility of the various names within a
/// particular module.
enum NameVisibilityKind {
Expand Down Expand Up @@ -596,6 +600,8 @@ class alignas(8) Module {
return Kind == ModuleInterfaceUnit || Kind == ModulePartitionInterface;
}

bool isNamedModuleInterfaceHasNoInit() const { return NamedModuleHasNoInit; }

/// Get the primary module interface name from a partition.
StringRef getPrimaryModuleInterfaceName() const {
// Technically, global module fragment belongs to global module. And global
Expand Down
2 changes: 1 addition & 1 deletion clang/lib/Basic/Module.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ Module::Module(StringRef Name, SourceLocation DefinitionLoc, Module *Parent,
InferSubmodules(false), InferExplicitSubmodules(false),
InferExportWildcard(false), ConfigMacrosExhaustive(false),
NoUndeclaredIncludes(false), ModuleMapIsPrivate(false),
NameVisibility(Hidden) {
NamedModuleHasNoInit(false), NameVisibility(Hidden) {
if (Parent) {
IsAvailable = Parent->isAvailable();
IsUnimportable = Parent->isUnimportable();
Expand Down
7 changes: 5 additions & 2 deletions clang/lib/CodeGen/CGDeclCXX.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -675,6 +675,10 @@ void CodeGenModule::EmitCXXModuleInitFunc(Module *Primary) {
// No Itanium initializer in header like modules.
if (M->isHeaderLikeModule())
continue; // TODO: warn of mixed use of module map modules and C++20?
// We're allowed to skip the initialization if we are sure it doesn't
// do any thing.
if (M->isNamedModuleInterfaceHasNoInit())
continue;
llvm::FunctionType *FTy = llvm::FunctionType::get(VoidTy, false);
SmallString<256> FnName;
{
Expand Down Expand Up @@ -731,8 +735,7 @@ void CodeGenModule::EmitCXXModuleInitFunc(Module *Primary) {
// If we have a completely empty initializer then we do not want to create
// the guard variable.
ConstantAddress GuardAddr = ConstantAddress::invalid();
if (!AllImports.empty() || !PrioritizedCXXGlobalInits.empty() ||
!CXXGlobalInits.empty()) {
if (!ModuleInits.empty()) {
// Create the guard var.
llvm::GlobalVariable *Guard = new llvm::GlobalVariable(
getModule(), Int8Ty, /*isConstant=*/false,
Expand Down
21 changes: 21 additions & 0 deletions clang/lib/Sema/Sema.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1245,6 +1245,27 @@ void Sema::ActOnEndOfTranslationUnit() {
}
}

// Now we can decide whether the modules we're building need an initializer.
if (Module *CurrentModule = getCurrentModule();
CurrentModule && CurrentModule->isInterfaceOrPartition()) {
auto DoesModNeedInit = [this](Module *M) {
if (!getASTContext().getModuleInitializers(M).empty())
return false;
for (auto [Exported, _] : M->Exports)
if (!Exported->isNamedModuleInterfaceHasNoInit())
return false;
for (Module *I : M->Imports)
if (!I->isNamedModuleInterfaceHasNoInit())
return false;

return true;
};

CurrentModule->NamedModuleHasNoInit = DoesModNeedInit(CurrentModule);
for (Module *SubModules : CurrentModule->submodules())
CurrentModule->NamedModuleHasNoInit &= DoesModNeedInit(SubModules);
Comment on lines +1265 to +1266
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be, maybe:

CurrentModule->NamedModuleHasNoInit = DoesModNeedInit(CurrentModule) && llvm::all_of(CurrentModule->submodules(), DoesModNeedInit);

(be slightly more efficient, since it'd bail out and return false as soon as one module needed init, rather than checking later modules)
Though the DoesModNeedInit might need a better name, since you'd expect that to return true if the module needs init, but the "no" in "noinit" is confusing/inverting - maybe it'd be better to name that flag "NamedModuleHasInit" - to avoid double negatives, etc?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the suggestion. It is addressed in the NFC patch: 7e8a0e4

}

// Warnings emitted in ActOnEndOfTranslationUnit() should be emitted for
// modules when they are built, not every time they are used.
emitAndClearUnusedLocalTypedefWarnings();
Expand Down
2 changes: 2 additions & 0 deletions clang/lib/Serialization/ASTReader.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5685,6 +5685,7 @@ llvm::Error ASTReader::ReadSubmoduleBlock(ModuleFile &F,
bool InferExportWildcard = Record[Idx++];
bool ConfigMacrosExhaustive = Record[Idx++];
bool ModuleMapIsPrivate = Record[Idx++];
bool NamedModuleHasNoInit = Record[Idx++];

Module *ParentModule = nullptr;
if (Parent)
Expand Down Expand Up @@ -5735,6 +5736,7 @@ llvm::Error ASTReader::ReadSubmoduleBlock(ModuleFile &F,
CurrentModule->InferExportWildcard = InferExportWildcard;
CurrentModule->ConfigMacrosExhaustive = ConfigMacrosExhaustive;
CurrentModule->ModuleMapIsPrivate = ModuleMapIsPrivate;
CurrentModule->NamedModuleHasNoInit = NamedModuleHasNoInit;
if (DeserializationListener)
DeserializationListener->ModuleRead(GlobalID, CurrentModule);

Expand Down
4 changes: 3 additions & 1 deletion clang/lib/Serialization/ASTWriter.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2779,6 +2779,7 @@ void ASTWriter::WriteSubmodules(Module *WritingModule) {
Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // InferExportWild...
Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // ConfigMacrosExh...
Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // ModuleMapIsPriv...
Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // NamedModuleHasN...
Abbrev->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Blob)); // Name
unsigned DefinitionAbbrev = Stream.EmitAbbrev(std::move(Abbrev));

Expand Down Expand Up @@ -2888,7 +2889,8 @@ void ASTWriter::WriteSubmodules(Module *WritingModule) {
Mod->InferExplicitSubmodules,
Mod->InferExportWildcard,
Mod->ConfigMacrosExhaustive,
Mod->ModuleMapIsPrivate};
Mod->ModuleMapIsPrivate,
Mod->NamedModuleHasNoInit};
Stream.EmitRecordWithBlob(DefinitionAbbrev, Record, Mod->Name);
}

Expand Down
53 changes: 41 additions & 12 deletions clang/test/CodeGenCXX/module-initializer-guard-elision.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,24 @@
// RUN: -o - | FileCheck %s --check-prefix=CHECK-O

// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 P.cpp \
// RUN: -emit-module-interface -fmodule-file=O.pcm -o P.pcm
// RUN: -emit-module-interface -fmodule-file=O=O.pcm -o P.pcm
// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 P.pcm -S -emit-llvm \
// RUN: -o - | FileCheck %s --check-prefix=CHECK-P

// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 Q.cpp \
// RUN: -emit-module-interface -fmodule-file=O.pcm -o Q.pcm
// RUN: -emit-module-interface -o Q.pcm
// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 Q.pcm -S -emit-llvm \
// RUN: -o - | FileCheck %s --check-prefix=CHECK-Q
// RUN: -o - | FileCheck %s --check-prefix=CHECK-Q

// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 R.cpp \
// RUN: -emit-module-interface -fmodule-file=Q=Q.pcm -o R.pcm
// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 R.pcm -S -emit-llvm \
// RUN: -o - | FileCheck %s --check-prefix=CHECK-R

// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 S.cpp \
// RUN: -emit-module-interface -fmodule-file=Q=Q.pcm -fmodule-file=R=R.pcm -o S.pcm
// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 S.pcm -S -emit-llvm \
// RUN: -o - | FileCheck %s --check-prefix=CHECK-S

// Testing cases where we can elide the module initializer guard variable.

Expand All @@ -31,8 +41,8 @@ export int foo ();
// CHECK-O-NEXT: ret void
// CHECK-O-NOT: @_ZGIW1O__in_chrg

// This has no global inits but imports a module, and therefore needs a guard
// variable.
// This has no global inits and all the imported modules don't need inits. So
// guard variable is not needed.
//--- P.cpp

export module P;
Expand All @@ -41,16 +51,14 @@ export import O;
export int bar ();

// CHECK-P: define void @_ZGIW1P
// CHECK-P-LABEL: init
// CHECK-P: store i8 1, ptr @_ZGIW1P__in_chrg
// CHECK-P: call void @_ZGIW1O()
// CHECK-P-NOT: call void @__cxx_global_var_init
// CHECK-P-LABEL: entry
// CHECK-P-NEXT: ret void
// CHECK-P-NOT: @_ZGIW1P__in_chrg

// This imports a module and has global inits, so needs a guard.
// This has global inits, so needs a guard.
//--- Q.cpp

export module Q;
export import O;

export struct Quack {
Quack(){};
Expand All @@ -64,6 +72,27 @@ export int baz ();
// CHECK-Q: call {{.*}} @_ZNW1Q5QuackC1Ev
// CHECK-Q: define void @_ZGIW1Q
// CHECK-Q: store i8 1, ptr @_ZGIW1Q__in_chrg
// CHECK-Q: call void @_ZGIW1O()
// CHECK-Q: call void @__cxx_global_var_init

// This doesn't have a global init, but it imports a module which needs global
// init, so needs a guard
//--- R.cpp

export module R;
export import Q;

// CHECK-R: define void @_ZGIW1R
// CHECK-R: store i8 1, ptr @_ZGIW1R__in_chrg
// CHECK-R: call{{.*}}@_ZGIW1Q

// This doesn't have a global init and the imported module doesn't have variables needs
// dynamic initialization.
// But the imported module contains modules initialization. So needs a guard.
//--- S.cpp

export module S;
export import R;

// CHECK-S: define void @_ZGIW1S
// CHECK-S: store i8 1, ptr @_ZGIW1S__in_chrg
// CHECK-S: call{{.*}}@_ZGIW1R