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