-
Notifications
You must be signed in to change notification settings - Fork 97
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[C++20] [Modules] Do we need the concept of key function
for class defined in module purview?
#170
Comments
I completely agree. I opened PR #171 to address this, as well as several related issues I saw while updating this section. |
Since #171 is merged, we can close this. |
of dynamic classes Close llvm#70585 and reflect itanium-cxx-abi/cxx-abi#170. The significant change of the patch is: for dynamic classes attached to module units, we generate the vtable to the attached module units directly and the key functions for such classes is meaningless.
of dynamic classes Close llvm#70585 and reflect itanium-cxx-abi/cxx-abi#170. The significant change of the patch is: for dynamic classes attached to module units, we generate the vtable to the attached module units directly and the key functions for such classes is meaningless.
Why does this not apply to all vague linkage entities defined in module purview? Shouldn't inline variables and functions also be emitted in the module unit that defines them? |
I think you're right that we could, at least for non-templated entities. It's an interesting question whether we should. If we eagerly emit a strong definition and then don't actually need it (e.g. because we inline every use of it), we'll end up having to hope the linker dead-strips it. The linker can probably do that reliably unless it's linking a shared library, in which case it probably can't do it at all. (Unfortunately, modules still don't tell us anything about how the module and its dependencies are assembled in the overall program, so default visibility still has to be the default.) On the other hand, of course, if we don't eagerly emit a strong definition, we're still stuck in the world of today where compilers waste huge amounts of time and energy emitting thousands of redundant definitions of inline functions into every translation unit, especially in unoptimized builds. But my sense is that most of that overhead in practice comes from templates, which modules don't help us with at all. I don't know. Changing the ABI here would be a huge change. It's probably our only shot to make a significant dent in the redundant emission problem, though. |
I feel this is a pretty complex topic. For a similar case, WG21 choose to make in-class member functions in module purview to be non implicitly inline. Also the inlined entities in module purview are helpful for optimizations. For this example:
Currently, the consumer of module Then for templates, maybe it will be an compile-time optimization to only emit the implicitly instantiated templates in the module unit. But we don't have a feeling how good will it be. From the perspective of compile-time optimization, maybe we should try to experiment this with a flag as an compiler extension to get a feeling. Then we can make further proposals. |
Indeed. I don't remember the rationale for that change.
I don't understand your point; an inline function can be emitted out-of-line and also still inlined.
The CMI could indicate which imported vague linkage entities were emitted in the module unit .o, so importers don't need to emit them as well? That would still require consistency between the compilations that produce the CMI and the .o, which might not be the same compiler invocation. |
What do you mean by emitting out-of-line? I didn't get this.
Yes, this is possible. There is a similar logic to handle non-inline entities. |
I assume Jason is talking about emitting a definition just for the benefit of local optimization. The idea is that inlining and function analysis should be able to see the definition, but if the compiler ultimately decides to emit a reference to it, it's just treated like an external symbol reference. In LLVM, this would be |
IIRC, module function bodies have reachability effects and implicit inline forced those wanting to control that to write implementations elsewhere in the file. This was decided in Cologne 2019. See paper P1779R0. |
Does the lack of those effects effectively force the use of unique emission, or is vague linkage still viable? |
Right, thanks. And under https://eel.is/c++draft/basic.link#17 it's ill-formed to refer to a TU-local entity in an exported inline definition, so the change was to allow more ABI isolation by not exporting the bodies of in-class member functions by default. I don't think this provides significant input into the current question, which has to do with functions (and variables) that are actually declared inline. As for templates, obviously all instantiations can't be emitted in the module unit, but any instantiations that are generated during compilation of the module unit could be. |
Yep. So originally when I implemented The data I got was related to @rjmccall's estimations: It's hard to know if/where this'll be a win or a loss. For So prototyping this with an opt-out for generated code like modules might be worth measuring and seeing how that looks. If that produced a positive outcome it'd point to maybe making this part of the ABI but with some opt-out that could be used by things like protobufs. The other problem that arises is optimized code - then even if the inline function is called, it might be inlined, so there's even less chance a homed definition will end up being needed. So in optimized builds it didn't work out so well. (oh, hey, I even gave an LLVM talk with some of the numbers in it: https://www.youtube.com/watch?v=lYYxDXgbUZ0 ) When @zygoloid implemented C++20 modules code generation, I think he opted them out of this homed-inline-functions feature, and I think I asked on the review but never learned exactly what motivated the change there. Though the unconvincing numbers are a pretty reasonable answer to that question, perhaps there was other data too. |
Interesting video, thanks. That does suggest that we probably don't want to mess with inline functions. But even putting inline functions aside, I think there's a strong case for treating inline variables the same as vtables. Possibly only inline variables that aren't usable in a constant expression, so references can't actually be inlined away. Also, any non-inline template instantiations that happen to be generated. |
I guess the reason may be that violates the ABI specification that time.
IIUC, do you want to say:
should have the same impact as
Do I understand correctly? If yes, I strongly suggest to send a paper to WG21 to forbid such uses if there is a strong motivation. Since it will be a big confusing to talk about the concept |
Previously, the following function bodies can be inlined into consumers via
But in a discussion in SG15 later, we think the behavior violates the ABI isolation. That said, we think the body of the non-inline function |
Are there precise rules about the ABI expectations with modules? I haven't been following this closely, and I really have no idea what's expected to be allowed to change and what isn't anymore. |
It is hard to tell concisely and precisely. Since in our discussion, we didn't image/expect to touch/change the ABI specification. My understanding of our consensus is: all the definitions of non-inline entities (functions and variables) can be excludes from the (abstract) interface part. Inline entities were exceptions in our previous discussion. I guess the major reason should be that we didn't expect to change the ABI specification. BTW, MSVC (I remember they don't follow Itanium C++ ABI) follows the same behavior: keep inline entities as exceptions. |
Okay. The committee needs to understand that that's not how this works, alright? Whenever the language adds new constructs, compilers have to figure out how to compile them, and that needs to go into this specification. Modules add quite a few new constructs, and they have non-trivial implications for compilers because they create new kinds of information flow between translation units. The traditional understanding of ABI boundaries in C++ has always been that any information in the translation unit is fair game for the compiler to use. A perhaps-naive understanding of how importing a module interface unit works is that the exported declarations from that module are now available in the importing translation unit essentially as if they were declared there (just preserving the knowledge that they in fact came from a different module). So if the idea is now that some of the information in a module interface unit is not supposed to be used by its importers, that's an important shift. That's true even if you've identified a reasonable formalism that seems to unify previous behavior with the new intended interpretation, because compilers need to make sure they're implementing the intent of that formalism. Let me try to restate the rule you're laying out so that we know we're in agreement. It sounds like the intended rule here is that the contents of function and variable definitions in the module interface unit are not part of the abstract interface of the module and should not be available to importers of the module unless the definition is explicitly marked
|
Thanks for write-up! I'll try to send this to SG15 to get a precise and formal expectation. (Also I want to clarify that what I said is a conclusion from the mailing list of SG15. And SG15 is tooling study group. Generally its outcome won't be merged into the
The current behavior (instead of calling it rule now) doesn't require it to be marked as explicitly inline. We'll drop the non-inline definition in the BMI (built module interface) so that the importers won't see the definition. (
)
Yes. This is the reason why we only discuss it in SG15 instead of EWG.
We didn't think about template definitions but template instantiations. So if the linkage of a template instantiation is not inline linkage (maybe this is what you call as This is possible via explicit template instantiations:
We think this is an important optimization technique with modules. For implicit instantiations, as far as I know, their linkages are inline linkages, so they won't fall into here.
Oh, interesting. I didn't test this. I just checked the current behavior in clang and it turns to that the rule doesn't apply to
We don't need to worry about semantical analyzing since all the information is preserved in the BMI.
The module implementation units are not special. They are treated as module consumers too. So if a non-inline definition in the module interface unit changed, it is allowed to not recompile to module implementation units.
I feel the word 'blocked' may not accurate. It is actual valid to recompile them. What we(SG15) want is to allow the behavior to skip the recompilations for the module consumers if only the non-inline definitions in module interface units change. But it is actually valid to recompile them all the time. In fact, currently the timestamp-based build systems will always recompile them if there is any change to the module interface unit. So they are a model in ideal. |
In that in both cases the module unit contains a definition that the importer can refer to instead of emitting its own, yes.
Forbid exported non-constant inline variables? I suppose they are pretty useless, and I would support warning about them, but forbidding them seems excessive. Anyway, I'm confident we don't want to forbid
How do I write my module such that A::table is available for constant evaluation, but an importer doesn't need to emit its own copy for any non-constant uses? That seems like the behavior that we would want, but there's no way to express it currently. Though I suppose we might revive the deprecated out-of-class redeclaration to mean that. Why treat A::table differently from a vtable? Of course, a plain int constexpr variable will usually be "inlined" away and the definition only needed if its address is taken, much like a simple inline function. But the cost of emitting a definition of such a variable should be small, and there are typically far fewer such variables than inline functions. |
Let me try to clarify.
will generate the definition of variable
won't generate the definition of variable Then you're proposing to warn for the second case?
Then you're saying Then you're proposing we should warn the use of explicitly inline variables in module purview and change the linkage of If yes, I still feel we can require the language to not make the currently It is not conflicting with constant evaluation or optimizations. Since with modules, we introduce a new layer called BMI. Then we can emit the definition of constexpr variables to the BMI so that the users of the BMI can see the definition. But we can choose to not generate its definition to the object files of its users (or we can generate it as
I didn't get the point of the paragraph. But I feel it may not be relevant any more. |
It's pretty common for classes doing complex bit-masking to build up their masks in In general, I feel it's pretty easy to distinguish v-tables from ordinary variables. If a dynamic class is used at all, its v-table will almost certainly have to be emitted; the compiler being able to completely optimize away any need for the v-table of an object is very uncommon in practice. Furthermore, v-tables tend to be somewhat large, and emitting one also requires the emission of RTTI and (usually) a significant number of inline virtual functions. It's certainly possible that all of this could also apply to an arbitrary variable — if nothing else, a user could create their own v-tables — but I think it's not unreasonable to say that programmers doing that ought to be expected to make smart decisions about how those variables will be emitted. We should use a simple, predictable rule here that still gives the programmer that control over emission. |
That would work for me, and would be consistent with the change in in-class function definitions. And for constexpr variables it's easier to separate the linkage from the usability-without-symbol-reference, since they correspond to different keywords, unlike with functions.
Yes. For both functions and constexpr variables, I want to have a way to get inlining without inline linkage, which it sounds like matches available_externally in Clang. The question is under what circumstances that linkage applies. I was suggesting above that the currently deprecated redeclaration at namespace scope (https://eel.is/c++draft/depr.static.constexpr) might be a suitable way to express it, if a bit cumbersome. Though (currently) that's not allowed for functions (https://godbolt.org/z/GrMMGzd8c). |
The model in my mind is, in
The definition of and the programmer can control the behavior by adding an
Then the definition of I feel it is straight forward, consistent and simple. WDYT?
IIUC, do you say it is not good to always generate the variable/vtable in the homing object eagerly? Then there is a simple question for vtables. In,
The vtable of |
BTW, I just summarized a draft paper to describe the requirement to ABI for modules: https://isocpp.org/files/papers/P3092R0.html. The discussion thread in SG15 is https://lists.isocpp.org/sg15/2024/01/2346.php. Since SG15 is public so you can chime in there if you have opinions on that. |
Currently, extern templates are detected by looking for the DECL_EXTERNAL flag on a TYPE_DECL. However, this is incorrect: TYPE_DECLs don't actually set this flag, and it happens to work by coincidence due to TYPE_DECL_SUPPRESS_DEBUG happening to use the same underlying bit. This however causes issues with other TYPE_DECLs that also happen to have suppressed debug information. Instead, this patch reworks the logic so CLASSTYPE_INTERFACE_ONLY is always emitted into the module BMI and can then be used to check for an extern template correctly. Otherwise, for other declarations we always want to redetermine this: even for declarations from the GMF, we may change our mind on whether to import or export depending on decisions made later in the TU after importing so we shouldn't decide this now, or necessarily reuse what the module we'd imported had decided. Some of this may need to change in the future to account for itanium-cxx-abi/cxx-abi#170. PR c++/112820 PR c++/102607 gcc/cp/ChangeLog: * module.cc (trees_out::lang_type_bools): Write interface_only and interface_unknown. (trees_in::lang_type_bools): Read the above flags. (trees_in::decl_value): Reset CLASSTYPE_INTERFACE_* except for extern templates. (trees_in::read_class_def): Remove buggy extern template handling. gcc/testsuite/ChangeLog: * g++.dg/modules/debug-2_a.C: New test. * g++.dg/modules/debug-2_b.C: New test. * g++.dg/modules/debug-2_c.C: New test. * g++.dg/modules/debug-3_a.C: New test. * g++.dg/modules/debug-3_b.C: New test. Signed-off-by: Nathaniel Shead <[email protected]>
of dynamic classes Close llvm#70585 and reflect itanium-cxx-abi/cxx-abi#170. The significant change of the patch is: for dynamic classes attached to module units, we generate the vtable to the attached module units directly and the key functions for such classes is meaningless.
Currently, extern templates are detected by looking for the DECL_EXTERNAL flag on a TYPE_DECL. However, this is incorrect: TYPE_DECLs don't actually set this flag, and it happens to work by coincidence due to TYPE_DECL_SUPPRESS_DEBUG happening to use the same underlying bit. This however causes issues with other TYPE_DECLs that also happen to have suppressed debug information. Instead, this patch reworks the logic so CLASSTYPE_INTERFACE_ONLY is always emitted into the module BMI and can then be used to check for an extern template correctly. Otherwise, for other declarations we always want to redetermine this: even for declarations from the GMF, we may change our mind on whether to import or export depending on decisions made later in the TU after importing so we shouldn't decide this now, or necessarily reuse what the module we'd imported had decided. Some of this may need to change in the future to account for itanium-cxx-abi/cxx-abi#170. PR c++/112820 PR c++/102607 gcc/cp/ChangeLog: * module.cc (trees_out::lang_type_bools): Write interface_only and interface_unknown. (trees_in::lang_type_bools): Read the above flags. (trees_in::decl_value): Reset CLASSTYPE_INTERFACE_* except for extern templates. (trees_in::read_class_def): Remove buggy extern template handling. gcc/testsuite/ChangeLog: * g++.dg/modules/debug-2_a.C: New test. * g++.dg/modules/debug-2_b.C: New test. * g++.dg/modules/debug-2_c.C: New test. * g++.dg/modules/debug-3_a.C: New test. * g++.dg/modules/debug-3_b.C: New test. Signed-off-by: Nathaniel Shead <[email protected]>
Currently, extern templates are detected by looking for the DECL_EXTERNAL flag on a TYPE_DECL. However, this is incorrect: TYPE_DECLs don't actually set this flag, and it happens to work by coincidence due to TYPE_DECL_SUPPRESS_DEBUG happening to use the same underlying bit. This however causes issues with other TYPE_DECLs that also happen to have suppressed debug information. Instead, this patch reworks the logic so CLASSTYPE_INTERFACE_ONLY is always emitted into the module BMI and can then be used to check for an extern template correctly. Otherwise, for other declarations we always want to redetermine this: even for declarations from the GMF, we may change our mind on whether to import or export depending on decisions made later in the TU after importing so we shouldn't decide this now, or necessarily reuse what the module we'd imported had decided. Some of this may need to change in the future to account for itanium-cxx-abi/cxx-abi#170. PR c++/112820 PR c++/102607 gcc/cp/ChangeLog: * module.cc (trees_out::lang_type_bools): Write interface_only and interface_unknown. (trees_in::lang_type_bools): Read the above flags. (trees_in::decl_value): Reset CLASSTYPE_INTERFACE_* except for extern templates. (trees_in::read_class_def): Remove buggy extern template handling. gcc/testsuite/ChangeLog: * g++.dg/modules/debug-2_a.C: New test. * g++.dg/modules/debug-2_b.C: New test. * g++.dg/modules/debug-2_c.C: New test. * g++.dg/modules/debug-3_a.C: New test. * g++.dg/modules/debug-3_b.C: New test. Signed-off-by: Nathaniel Shead <[email protected]>
Currently, extern templates are detected by looking for the DECL_EXTERNAL flag on a TYPE_DECL. However, this is incorrect: TYPE_DECLs don't actually set this flag, and it happens to work by coincidence due to TYPE_DECL_SUPPRESS_DEBUG happening to use the same underlying bit. This however causes issues with other TYPE_DECLs that also happen to have suppressed debug information. Instead, this patch reworks the logic so CLASSTYPE_INTERFACE_ONLY is always emitted into the module BMI and can then be used to check for an extern template correctly. Otherwise, for other declarations we always want to redetermine this: even for declarations from the GMF, we may change our mind on whether to import or export depending on decisions made later in the TU after importing so we shouldn't decide this now, or necessarily reuse what the module we'd imported had decided. Some of this may need to change in the future to account for itanium-cxx-abi/cxx-abi#170. PR c++/112820 PR c++/102607 gcc/cp/ChangeLog: * module.cc (trees_out::lang_type_bools): Write interface_only and interface_unknown. (trees_in::lang_type_bools): Read the above flags. (trees_in::decl_value): Reset CLASSTYPE_INTERFACE_* except for extern templates. (trees_in::read_class_def): Remove buggy extern template handling. gcc/testsuite/ChangeLog: * g++.dg/modules/debug-2_a.C: New test. * g++.dg/modules/debug-2_b.C: New test. * g++.dg/modules/debug-2_c.C: New test. * g++.dg/modules/debug-3_a.C: New test. * g++.dg/modules/debug-3_b.C: New test. Signed-off-by: Nathaniel Shead <[email protected]>
Currently, extern templates are detected by looking for the DECL_EXTERNAL flag on a TYPE_DECL. However, this is incorrect: TYPE_DECLs don't actually set this flag, and it happens to work by coincidence due to TYPE_DECL_SUPPRESS_DEBUG happening to use the same underlying bit. This however causes issues with other TYPE_DECLs that also happen to have suppressed debug information. Instead, this patch reworks the logic so CLASSTYPE_INTERFACE_ONLY is always emitted into the module BMI and can then be used to check for an extern template correctly. Otherwise, for other declarations we always want to redetermine this: even for declarations from the GMF, we may change our mind on whether to import or export depending on decisions made later in the TU after importing so we shouldn't decide this now, or necessarily reuse what the module we'd imported had decided. Some of this may need to change in the future to account for itanium-cxx-abi/cxx-abi#170. PR c++/112820 PR c++/102607 gcc/cp/ChangeLog: * module.cc (trees_out::lang_type_bools): Write interface_only and interface_unknown. (trees_in::lang_type_bools): Read the above flags. (trees_in::decl_value): Reset CLASSTYPE_INTERFACE_* except for extern templates. (trees_in::read_class_def): Remove buggy extern template handling. gcc/testsuite/ChangeLog: * g++.dg/modules/debug-2_a.C: New test. * g++.dg/modules/debug-2_b.C: New test. * g++.dg/modules/debug-2_c.C: New test. * g++.dg/modules/debug-3_a.C: New test. * g++.dg/modules/debug-3_b.C: New test. Signed-off-by: Nathaniel Shead <[email protected]>
Currently, extern templates are detected by looking for the DECL_EXTERNAL flag on a TYPE_DECL. However, this is incorrect: TYPE_DECLs don't actually set this flag, and it happens to work by coincidence due to TYPE_DECL_SUPPRESS_DEBUG happening to use the same underlying bit. This however causes issues with other TYPE_DECLs that also happen to have suppressed debug information. Instead, this patch reworks the logic so CLASSTYPE_INTERFACE_ONLY is always emitted into the module BMI and can then be used to check for an extern template correctly. Otherwise, for other declarations we always want to redetermine this: even for declarations from the GMF, we may change our mind on whether to import or export depending on decisions made later in the TU after importing so we shouldn't decide this now, or necessarily reuse what the module we'd imported had decided. Some of this may need to change in the future to account for itanium-cxx-abi/cxx-abi#170. PR c++/112820 PR c++/102607 gcc/cp/ChangeLog: * module.cc (trees_out::lang_type_bools): Write interface_only and interface_unknown. (trees_in::lang_type_bools): Read the above flags. (trees_in::decl_value): Reset CLASSTYPE_INTERFACE_* except for extern templates. (trees_in::read_class_def): Remove buggy extern template handling. gcc/testsuite/ChangeLog: * g++.dg/modules/debug-2_a.C: New test. * g++.dg/modules/debug-2_b.C: New test. * g++.dg/modules/debug-2_c.C: New test. * g++.dg/modules/debug-3_a.C: New test. * g++.dg/modules/debug-3_b.C: New test. Signed-off-by: Nathaniel Shead <[email protected]>
Currently, extern templates are detected by looking for the DECL_EXTERNAL flag on a TYPE_DECL. However, this is incorrect: TYPE_DECLs don't actually set this flag, and it happens to work by coincidence due to TYPE_DECL_SUPPRESS_DEBUG happening to use the same underlying bit. This however causes issues with other TYPE_DECLs that also happen to have suppressed debug information. Instead, this patch reworks the logic so CLASSTYPE_INTERFACE_ONLY is always emitted into the module BMI and can then be used to check for an extern template correctly. Otherwise, for other declarations we always want to redetermine this: even for declarations from the GMF, we may change our mind on whether to import or export depending on decisions made later in the TU after importing so we shouldn't decide this now, or necessarily reuse what the module we'd imported had decided. Some of this may need to change in the future to account for itanium-cxx-abi/cxx-abi#170. PR c++/112820 PR c++/102607 gcc/cp/ChangeLog: * module.cc (trees_out::lang_type_bools): Write interface_only and interface_unknown. (trees_in::lang_type_bools): Read the above flags. (trees_in::decl_value): Reset CLASSTYPE_INTERFACE_* except for extern templates. (trees_in::read_class_def): Remove buggy extern template handling. gcc/testsuite/ChangeLog: * g++.dg/modules/debug-2_a.C: New test. * g++.dg/modules/debug-2_b.C: New test. * g++.dg/modules/debug-2_c.C: New test. * g++.dg/modules/debug-3_a.C: New test. * g++.dg/modules/debug-3_b.C: New test. Signed-off-by: Nathaniel Shead <[email protected]>
of dynamic classes Close llvm#70585 and reflect itanium-cxx-abi/cxx-abi#170. The significant change of the patch is: for dynamic classes attached to module units, we generate the vtable to the attached module units directly and the key functions for such classes is meaningless.
of dynamic classes Close llvm#70585 and reflect itanium-cxx-abi/cxx-abi#170. The significant change of the patch is: for dynamic classes attached to module units, we generate the vtable to the attached module units directly and the key functions for such classes is meaningless.
of dynamic classes Close llvm#70585 and reflect itanium-cxx-abi/cxx-abi#170. The significant change of the patch is: for dynamic classes attached to module units, we generate the vtable to the attached module units directly and the key functions for such classes is meaningless.
…t of dynamic classes (#75912) Close #70585 and reflect itanium-cxx-abi/cxx-abi#170. The significant change of the patch is: for dynamic classes attached to module units, we generate the vtable to the attached module units directly and the key functions for such classes is meaningless.
…ule unit of dynamic classes (llvm#75912) Close llvm#70585 and reflect itanium-cxx-abi/cxx-abi#170. The significant change of the patch is: for dynamic classes attached to module units, we generate the vtable to the attached module units directly and the key functions for such classes is meaningless.
See llvm/llvm-project#70585 for the motivation issue.
Simply, the current clang's behavior violates itanium ABI 5.2.3:
But there are opinions about: is the concept of key function necessary after we introducing modules? So I bring the issue here.
My thought about the key function is that: in the headers model, the class may be defined in many TUs, then we need to find a TU to generate the virtual table. But after we introducing modules, if we define the class is defined is a module purview, we can get the TU to generate the virtual table naturally.
My understanding to the issues is: while the original old rule works, maybe it is better to have some new rules with the new features technically.
The text was updated successfully, but these errors were encountered: