[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/lib/CodeGen/CGDeclCXX.cpp:621
 
+void CodeGenModule::EmitCXXModuleInitFunc(Module *Primary) {
+  while (!CXXGlobalInits.empty() && !CXXGlobalInits.back())
----------------
iains wrote:
> ChuanqiXu wrote:
> > Recommend to comment related part in your summary. It should be much helpful for other to read the codes.
> Hmm. I am not sure exactly what you mean here - there is a comment about the purpose of the method, where the method is declared.
> The commit message describes what this method does in the first part.  I'm happy to make things more clear, but not sure where you want to see some change,
I mean the paragraph:
```
For a module (instead of the generic CXX initializer) we emit a module init
that:

- wraps the contained initializations in a control variable to ensure that the inits only happen once, even if a module is imported many times by imports of the main unit.
- calls module initialisers for imported modules first. Note that the order of module import is not significant, and therefore neither is the order of imported module initializers.
- 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)
```

I understand people might feel like this is wordy. But, **personally**, I prefer readability.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:645-646
+      llvm::raw_svector_ostream Out(FnName);
+      cast<ItaniumMangleContext>(getCXXABI().getMangleContext())
+          .mangleModuleInitializer(M, Out);
+    }
----------------
iains wrote:
> ChuanqiXu wrote:
> > mangleModuleInitializer is virtual function and we don't need to cast it.
> Actually, 'mangleModuleInitializer' is not a virtual function of the 'MangleContext' class, only of the 'ItaniumMangleContext'.
> (see D122741).
> 
> This because we do not know the mangling for MS and, in principle, there might never be such a mangling (e.g. that implementation might deal with P1874 in some different way).
> 
> I did have a version of this where I amended the mangling to make the function virtual in the 'MangleContext' class, but then that means generating a dummy MS version that, as noted above, might never exist in reality.
> 
I was just afraid of people might blame us to bring ABI specific things to CodeGen* things. I found there similar uses in CGVTables.cpp and CGVTT.cpp. So this might be acceptable too.


================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:671
+        ModuleInits.push_back(I->second);
+    }
+    PrioritizedCXXGlobalInits.clear();
----------------
iains wrote:
> ChuanqiXu wrote:
> > 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?
> In the general CXX initializer the prioritized inits are grouped into functions named in a way that allows them to be collated by other tools (e.g. the static linker) so that init priority can be honoured over multiple TUs. 
> 
> This is not feasible for module initializers, there is no way to fuze them across TUs (it would have no meaning).  So I think all we need to do here is to emit the calls in the correct order (in the end there are no more initializer functions, we just skip the intermediated grouping functions,
> 
> Of course, init priority is not mentioned in the standard, so that really what would matter is that compiler 'vendors' agree on a sensible approach to make sure code behaves in an expected manner for common toolchains.
Yeah, the prioritized inits are really odd fashion to me..  I only saw such things in assembly. So my understanding to this one is that:
(1) If we have prioritized inits in modules' code , it is not guaranteed to be initialized first. This is the vendors agreement.
(2) If we link a static library (e.g., libgcc.a), the prioritized inits in that would still be initialized first.

Do I understand right?


================
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)) {
----------------
iains wrote:
> ChuanqiXu wrote:
> > 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.
> yeah, I was intending to do this actually - although it only moves the magic strings to a different place.  It might be that we have enough bits in the module type enumerator (already streamed) so that we could use up two values with "GMF" and "PMF" module types.  However IMO that should be a separate change.
We still need to use `auto *` instead of `auto` here. I remember this is recommended in clang's coding standards.


================
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)) {
----------------
ChuanqiXu wrote:
> Nit: with above: `Module *Module::getPrivateModuleFragment()`.
Use auto* here.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3206-3208
+  bool MBEE = MayBeEmittedEagerly(Global);
+  if (MustBeEmitted(Global)) {
+    if (MBEE) {
----------------
iains wrote:
> ChuanqiXu wrote:
> > Is this change needed? Could you elaborate more on this?
> the test is quite heavy, and is used twice when the assertions are enabled (I do not mind to revert this part if that does not seem worthwhile).
> 
I prefer to do such changes in a standalone NFC patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126189