[C++20][Modules] Elide unused guard variables in Itanium ABI module initializers.

[iains]

Summary

For the Itanium ABI, we emit an initializer for each module. This is responsible
for handling initialization of global vars. Relates to P1874R1.

The initializer has a known mangling and is automatically called from any TU that
imports a module. Since, at present, the importer has no way to determine that an
imported module does not require an initializer, we generate the initializer for
all cases (even when it is empty).

Initializers must be run once, with the ordering guaranteed by the import graph
and this is ensured in the current code by addition of a guard variable.

In the case that a module has no requirement for global initializers, and also does
not import any other modules, we can elide the guard variable.

[dblaikie]

clang/lib/CodeGen/CGDeclCXX.cpp

690–695

Usually I'd expect this to be sunk down closer to its usage - any particular reason to have it up here? (not super far away, but seems further away than I'd expect)

730

Could we avoid duplicating this call, by doing something like this:

ConstantAddress Guard = ConstantAddress::invalid()
if (!PrioritizedCXXGlobalInits.empty() || !CXXGlobalInits.empty())
  Guard = ConstantAddress(new GlobalVariable(...), Int8Ty, GuardAlign);
CodeGenFunction(*this).GenerateCXXGlobalInitFunc(Fn, ModuleInits, Guard);

?

clang/test/CodeGenCXX/module-initializer-guard-elision.cpp

23–44

If it's pretty stable/reliable/not too brittle, maybe worth CHECKing the whole function in these cases (CHECK+CHECK-NEXT etc) to demonstrate they have nothing in them other than the return, or the call+return?

[iains]

address review comments, minor amendments to description.

[iains]

thanks for the review.

clang/lib/CodeGen/CGDeclCXX.cpp

690–695

Agreed.
The reason was that we build an ordered vector of inits from several potential sources, and I'd been clearing the inputs as they were used. I've now sunk those clears to the end of the function and merged the test as you suggest below.

730

works for me.

clang/test/CodeGenCXX/module-initializer-guard-elision.cpp

23–44

The main thing we care about is that there is no guard variable - however it does no harm to be specific where we can, For the empty/nearly empty functions, that seems reasonable (done for those).

[urnathan]

Richard & I discussed this, and decided it was a bad idea, as it means we could have an O(N^2) initialization walk, rather than O(N).

IIUC this is being motivated by NVPTX, which cannot support dynamically initialized global vars. But in that case we know all the global init fns must be NOP. However, I recommend not solving that problem piecemeal, but involve NVidia more directly. (There is an alternative design, relying on section ordering, but it is more brittle, and IIRC still requires idempotency vars in some cases.) We should not be pessimizing systems that have dynamic inits to cater for systems that lack them.

For GCC I (eventually) implemented optimizations where:
1 each CMI indicates whether it has a NOP global init fn.
2 the global init was always emitted, but if it is NOP, it's a completely empty fn (thus importers are not required to implement the below optimization)
3 Importers do not emit calls to known-NOP global inits.

Step #1 took a bit of moving things around, as one only knew there were no global inits at a later point than the CMI was being written out.

[iains]

So, in light of these comments;

• It seems that there is one case where I can elide the guard - where the init is completely empty (including of calls to imported module inits)
• otherwise, I need to investigate what is needed for an importee to communicate that it has an empty init and thus does not need to be called.

I think the first of these could be useful in quite a few cases (it's not clear to me at this relatively early stage in modules adoption that we see very deep include graphs - but somewhat unknown).

The second will have to live on the TODO for now (at least from me).

[iains]

rebased and updated to elide the guard only for the case of no inits _and_ no imports.

[urnathan]

This is ok, but see the note I added about object-file compatibility.

clang/lib/CodeGen/CGDeclCXX.cpp

646–647

It's not just 'current implementation' requirement, it's an ABI requirement. Remember, one could generate the interface object file from one compiler and then generate (and consume) the CMIs with a different compiler. this achieves object-file interoperability, but does not require CMI compatibility. We have no expectation any particular compiler implements the optimization you refer to.

Feel free to note this at your discretion.

[urnathan]

It occurs to me that for systems that lack dynamic initialization (like PTX), it is trivial to implement the 'don't call NOP module inits', because we know that there can be no non-nop inits. Any code emission that tries to create a global init fn with contents will fail to compile.

[iains]

rebased, amended a comment as suggested

clang/lib/CodeGen/CGDeclCXX.cpp

void CodeGenModule::EmitCXXThreadLocalInitFunc() {
   CXXThreadLocals.clear();
 }
 
 /* Build the initializer for a C++20 module:
    This is arranged to be run only once regardless of how many times the module
    might be included transitively.  This arranged by using a control variable.
 
    First we call any initializers for imported modules.
    We then call initializers for the Global Module Fragment (if present)
    We then call initializers for the current module.
    We then call initializers for the Private Module Fragment (if present)
 */
 
 void CodeGenModule::EmitCXXModuleInitFunc(Module *Primary) {
   while (!CXXGlobalInits.empty() && !CXXGlobalInits.back())
     CXXGlobalInits.pop_back();
 
   // We create the function, even if it is empty, since an importer of this
for (Module *M : AllImports) {
     assert(!GetGlobalValue(FnName.str()) &&
            "We should only have one use of the initializer call");
     llvm::Function *Fn = llvm::Function::Create(
         FTy, llvm::Function::ExternalLinkage, FnName.str(), &getModule());
     ModuleInits.push_back(Fn);
   }
   AllImports.clear();
 
   // Add any initializers with specified priority; this uses the same  approach
   // as EmitCXXGlobalInitFunc().
   if (!PrioritizedCXXGlobalInits.empty()) {
     SmallVector<llvm::Function *, 8> LocalCXXGlobalInits;
     llvm::array_pod_sort(PrioritizedCXXGlobalInits.begin(),
                          PrioritizedCXXGlobalInits.end());
     for (SmallVectorImpl<GlobalInitData>::iterator
              I = PrioritizedCXXGlobalInits.begin(),
              E = PrioritizedCXXGlobalInits.end();
          I != E;) {
       SmallVectorImpl<GlobalInitData>::iterator PrioE =
           std::upper_bound(I + 1, E, *I, GlobalInitPriorityCmp());
 
       for (; I < PrioE; ++I)
         ModuleInits.push_back(I->second);
     }
-    PrioritizedCXXGlobalInits.clear();
   }
 
   // Now append the ones without specified priority.
   for (auto *F : CXXGlobalInits)
     ModuleInits.push_back(F);
-  CXXGlobalInits.clear();
 
   llvm::FunctionType *FTy = llvm::FunctionType::get(VoidTy, false);
   const CGFunctionInfo &FI = getTypes().arrangeNullaryFunction();
 
   // We now build the initializer for this module, which has a mangled name
   // as per the Itanium ABI .  The action of the initializer is guarded so that
   // each init is run just once (even though a module might be imported
   // multiple times via nested use).
   llvm::Function *Fn;
-  llvm::GlobalVariable *Guard = nullptr;
   {
     SmallString<256> InitFnName;
     llvm::raw_svector_ostream Out(InitFnName);
     cast<ItaniumMangleContext>(getCXXABI().getMangleContext())
         .mangleModuleInitializer(Primary, Out);
     Fn = CreateGlobalInitOrCleanUpFunction(
         FTy, llvm::Twine(InitFnName), FI, SourceLocation(), false,
         llvm::GlobalVariable::ExternalLinkage);
 
-    Guard = new llvm::GlobalVariable(getModule(), Int8Ty, /*isConstant=*/false,
-                                     llvm::GlobalVariable::InternalLinkage,
-                                     llvm::ConstantInt::get(Int8Ty, 0),
-                                     InitFnName.str() + "__in_chrg");
-  }
+    // If we have an empty initializer then we do not want to create a guard
+    // var.  'Empty' excludes calls to imported module initializers since
+    // those will contain local guards as needed (and might themselves
+    // be empty).
+    ConstantAddress GuardAddr = ConstantAddress::invalid();
+    if (!PrioritizedCXXGlobalInits.empty() || !CXXGlobalInits.empty()) {
+      // Create the guard var.
+      llvm::GlobalVariable *Guard = new llvm::GlobalVariable(
+          getModule(), Int8Ty, /*isConstant=*/false,
+          llvm::ConstantInt::get(Int8Ty, 0), InitFnName.str() + "__in_chrg");
       CharUnits GuardAlign = CharUnits::One();
       Guard->setAlignment(GuardAlign.getAsAlign());
+      GuardAddr = ConstantAddress(Guard, Int8Ty, GuardAlign);
+    }
+    CodeGenFunction(*this).GenerateCXXGlobalInitFunc(Fn, ModuleInits,
+                                                     GuardAddr);
+  }
 
-  CodeGenFunction(*this).GenerateCXXGlobalInitFunc(
-      Fn, ModuleInits, ConstantAddress(Guard, Int8Ty, GuardAlign));
-  // We allow for the case that a module object is added to  a linked binary
+  // We allow for the case that a module object is added to a linked binary
   // without a specific call to the the initializer.  This also ensure that
   // implementation partition initializers are called when the partition
   // is not imported as an interface.
   AddGlobalCtor(Fn);
 
   // See the comment in EmitCXXGlobalInitFunc about OpenCL global init
   // functions.
   if (getLangOpts().OpenCL) {
     GenKernelArgMetadata(Fn);
     Fn->setCallingConv(llvm::CallingConv::SPIR_KERNEL);
   }
 
   assert(!getLangOpts().CUDA || !getLangOpts().CUDAIsDevice ||
          getLangOpts().GPUAllowDeviceInit);
   if (getLangOpts().HIP && getLangOpts().CUDAIsDevice) {
     Fn->setCallingConv(llvm::CallingConv::AMDGPU_KERNEL);
     Fn->addFnAttr("device-init");
   }
 
+  // We are done with the inits.
+  PrioritizedCXXGlobalInits.clear();
+  CXXGlobalInits.clear();
   ModuleInits.clear();
 }
 
 static SmallString<128> getTransformedFileName(llvm::Module &M) {
   SmallString<128> FileName = llvm::sys::path::filename(M.getName());
 
   if (FileName.empty())
     FileName = "<null>";

clang/test/CodeGenCXX/module-initializer-guard-elision.cpp

+// RUN: rm -rf %t
+// RUN: split-file %s %t
+// RUN: cd %t
+
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 O.cpp \
+// RUN:    -emit-module-interface -o O.pcm
+// RUN: %clang_cc1 -triple %itanium_abi_triple -std=c++20 O.pcm -S -emit-llvm \
+// 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: %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: %clang_cc1 -triple %itanium_abi_triple -std=c++20 Q.pcm -S -emit-llvm \
+// RUN:  -o - | FileCheck %s --check-prefix=CHECK-Q
+
+// Testing cases where we can elide the module initializer guard variable.
+
+// This module has no global inits and does not import any other module
+//--- O.cpp
+
+export module O;
+
+export int foo ();
+
+// CHECK-O: define void @_ZGIW1O
+// CHECK-O-LABEL: entry
+// CHECK-O-NEXT: ret void
+// CHECK-O-NOT: @_ZGIW1O__in_chrg
+
+// This has no global inits but imports a module.
+//--- P.cpp
+
+export module P;
+
+export import O;
+export int bar ();
+
+// CHECK-P: define void @_ZGIW1P
+// CHECK-P-LABEL: entry
+// CHECK-P-NEXT: call void @_ZGIW1O()
+// CHECK-P-NEXT: ret void
+// CHECK-P-NOT: @_ZGIW1P__in_chrg
+
+// This imports a module and has global inits, so needs a guard.
+//--- Q.cpp
+
+export module Q;
+export import O;
+
+export struct Quack {
+  Quack(){};
+};
+
+export Quack Duck;
+
+export int baz ();
+
+// CHECK-Q: define internal void @__cxx_global_var_init
+// 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
+