[PATCH] D126189: [C++20][Modules] Build module static initializers per P1874R1.

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu added inline comments.


================
Comment at: clang/include/clang/AST/ASTContext.h:476
+  /// For module code-gen cases, this is the top-level module we are building.
+  mutable Module *PrimaryModule = nullptr;
+
----------------
The name `PrimaryModule` looks confusing. PrimaryModule looks like primary module interface unit to me. I think it refers to current module unit only. I think it is better to rename to `CurrentModuleUnit`.

Then why is it mutable? I don't find it is changed in a const member function.


================
Comment at: clang/include/clang/AST/ASTContext.h:1085
+  /// Get module under construction, nullptr if this is not a C++20 module.
+  Module *getModuleForCodeGen() { return PrimaryModule; }
+
----------------



================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:621
 
+void CodeGenModule::EmitCXXModuleInitFunc(Module *Primary) {
+  while (!CXXGlobalInits.empty() && !CXXGlobalInits.back())
----------------
Recommend to comment related part in your summary. It should be much helpful for other to read the codes.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:645-646
+      llvm::raw_svector_ostream Out(FnName);
+      cast<ItaniumMangleContext>(getCXXABI().getMangleContext())
+          .mangleModuleInitializer(M, Out);
+    }
----------------
mangleModuleInitializer is virtual function and we don't need to cast it.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:671
+        ModuleInits.push_back(I->second);
+    }
+    PrioritizedCXXGlobalInits.clear();
----------------
I find the process here for `PrioritizedCXXGlobalInits` is much simpler than the one done in `CodeGenModule::EmitCXXGlobalInitFunc()`. It would generate more global init function. Doesn't it matter?


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:684
+  // We now build the initialiser 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
----------------
Given CodeGenModule is designed as ABI independent, I feel better to avoid see Itanium ABI in this function. (Although we don't have other ABI for modules now..)


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:692-693
+    llvm::raw_svector_ostream Out(InitFnName);
+    cast<ItaniumMangleContext>(getCXXABI().getMangleContext())
+        .mangleModuleInitializer(getContext().getModuleForCodeGen(), Out);
+    Fn = CreateGlobalInitOrCleanUpFunction(
----------------



================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:699
+    Guard = new llvm::GlobalVariable(getModule(), Int8Ty, /*isConstant=*/false,
+                                     llvm::GlobalVariable::InternalLinkage,
+                                     llvm::ConstantInt::get(Int8Ty, 0),
----------------
Should the Guard be internal linkage? I image that it is possible to be manipulated by different TUs. So I feel like it might be better to be linkonce or linkonce_odr?


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:706-722
+  CodeGenFunction(*this).GenerateCXXGlobalInitFunc(
+      Fn, ModuleInits, ConstantAddress(Guard, Int8Ty, GuardAlign));
+  AddGlobalCtor(Fn);
+
+  // See the comment in EmitCXXGlobalInitFunc about OpenCL global init
+  // functions.
+  if (getLangOpts().OpenCL) {
----------------
The codes look highly similar to CodeGenModule::EmitCXXGlobalInitFunc. I feel it is better to hoist a new function to avoid repeating the same logic twice.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:846
+
+  CodeGenFunction(*this).GenerateCXXGlobalInitFunc(Fn, ModuleInits);
   AddGlobalCtor(Fn);
----------------
This looks odd since it might be possible to not handling modules. But I understand it might be time-consuming if we want to use `CXXGloablInits` and we want `ModuleInits` to be front.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2489
+  // source, first Global Module Fragments, if present.
+  if (auto GMF = Primary->findSubmodule("<global>")) {
+    for (Decl *D : getContext().getModuleInitializers(GMF)) {
----------------
Nit: I feel better to add methods like `Module *Module::getGlobalModuleFragment()`. I feel odd to use magic string here. But I am OK with it since there many magic string in CodeGen codes and we got in consensus before that we would need to do a lot of refactor work in the future.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2503
+  // Third any associated with the Privat eMOdule Fragment, if present.
+  if (auto PMF = Primary->findSubmodule("<private>")) {
+    for (Decl *D : getContext().getModuleInitializers(PMF)) {
----------------
Nit: with above: `Module *Module::getPrivateModuleFragment()`.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:2933-2935
+      // llvm::dbgs() << "deferring: ";
+      // VD->dump();
+      return false;
----------------



================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3206-3208
+  bool MBEE = MayBeEmittedEagerly(Global);
+  if (MustBeEmitted(Global)) {
+    if (MBEE) {
----------------
Is this change needed? Could you elaborate more on this?


================
Comment at: clang/lib/CodeGen/CodeGenModule.h:1531
+  /// Emit the function that initializes global variables for a C++ Module.
+  void EmitCXXModuleInitFunc(Module *Primary);
+
----------------
Let's keep consistent with the following. Given CodeGen part uses LLVM IR tensely, `clang::Module` looks better than `Module*`


================
Comment at: clang/test/CodeGen/module-intializer.cpp:56
+// CHECK-N: define void @_ZGIW1N
+// CHECK-N: store i8 1, ptr @_ZGIW1N__in_chrg
+// CHECK-N: call void @__cxx_global_var_init
----------------
I think it is necessary to check the load for the guard and the check.


================
Comment at: clang/test/CodeGen/module-intializer.cpp:172
+// CHECK-USE: define internal void @_GLOBAL__sub_I_useM.cpp
+// CHECK-USE: call void @_ZGIW1M()
----------------
It looks like we lack a test for PrivateModuleFragment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D126189/new/

https://reviews.llvm.org/D126189