[PATCH] D126189: [C++20][Modules] Build module static initializers per P1874R1.
Iain Sandoe via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu May 26 04:03:45 PDT 2022
iains added a comment.
not sure why the Debian clang-format is complaining - I am using llvm-14's clang-format with git clang-format + arc.
================
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;
+
----------------
ChuanqiXu wrote:
> 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.
> 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`.
I think we want to be clear that this is the Top Level Module (and not some dependent or sub-module) - so I have renamed to "TopLevelModule".
> Then why is it mutable? I don't find it is changed in a const member function.
Ah well caught; the implementation went through several iterations and I missed to remove this.
================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:621
+void CodeGenModule::EmitCXXModuleInitFunc(Module *Primary) {
+ while (!CXXGlobalInits.empty() && !CXXGlobalInits.back())
----------------
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,
================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:626-627
+ // We create the function, even if it is empty, since an importer of this
+ // module will refer to it unconditionally (there is no way for an importer
+ // to know if the function could be omitted at this time).
+
----------------
urnathan wrote:
> 'at this time' is ambiguous. I think it means 'the current compiler implementation state', but it could also mean 'at this point in the compilation'. I don't think there's a general problem with such an optimization -- we could emit a flag into the BMI. It's just we don't have the smarts to do that just yet. right?
yeah, I meant "as currently implemented" - adding flags to the BMI attracts concomitant churn in the serialisation, so probably better done separately. Amended the comment.
================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:629
+
+ // Module initialisers for imported modules are emitted first.
+ // Collect the modules that we import
----------------
urnathan wrote:
> I'm with Morse about this, even before coming to live where I do. You've used both 'ise' and 'ize'. s/ise/ize/
tools tools ... my editor wants "ise" unless I remember to set it to USian .. and sometimes I miss a change,, hopefully will catch them all as I go.
================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:645-646
+ llvm::raw_svector_ostream Out(FnName);
+ cast<ItaniumMangleContext>(getCXXABI().getMangleContext())
+ .mangleModuleInitializer(M, Out);
+ }
----------------
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.
================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:671
+ ModuleInits.push_back(I->second);
+ }
+ PrioritizedCXXGlobalInits.clear();
----------------
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.
================
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
----------------
ChuanqiXu wrote:
> 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..)
Until we have an answer on MS's approach to this, I think it is better to be clear that this only applies to Itanium ABI uses (but if consensus is to remove this, I can do so).
================
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(
----------------
ChuanqiXu wrote:
>
we need the cast, see above.
================
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) {
----------------
ChuanqiXu wrote:
> 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.
actually, I had considered trying to factor this a bit, but it looks kind of artificial (will consider a second iteration).
================
Comment at: clang/lib/CodeGen/CGDeclCXX.cpp:846
+
+ CodeGenFunction(*this).GenerateCXXGlobalInitFunc(Fn, ModuleInits);
AddGlobalCtor(Fn);
----------------
ChuanqiXu wrote:
> 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.
Yes - otherwise we end up inserting a bunch of stuff, the code is not too long, and the pre-pending is described at the stage it is done.
================
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)) {
----------------
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.
================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3206-3208
+ bool MBEE = MayBeEmittedEagerly(Global);
+ if (MustBeEmitted(Global)) {
+ if (MBEE) {
----------------
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).
================
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
----------------
ChuanqiXu wrote:
> I think it is necessary to check the load for the guard and the check.
can you say why?
the main purpose here is to check that we have the right mangled symbols (and some check that there is useful ordering). In what case would you think code-gen could emit the store and somehow not the other parts?
================
Comment at: clang/test/CodeGen/module-intializer.cpp:172
+// CHECK-USE: define internal void @_GLOBAL__sub_I_useM.cpp
+// CHECK-USE: call void @_ZGIW1M()
----------------
ChuanqiXu wrote:
> It looks like we lack a test for PrivateModuleFragment.
yeah, needs to be a second test since we cannot have PMF in a module with partitions.
added one.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126189/new/
https://reviews.llvm.org/D126189
More information about the cfe-commits
mailing list