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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
