<div dir="ltr">Ping</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Sep 21, 2021 at 7:58 PM David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Ping</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Sep 5, 2021 at 11:28 AM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hey Richard - was just going back over some of the modular codegen code (due to a discussion on the EWG mailing list about file extensions that ended up touching on the nature of how modules are built) - and I came across this code & had the same question I see I wrote up here already but got lost in the *-commits mail.<br><br>Wondering if you've got some thoughts on why this choice was implemented for C++20 modules - not homing inline functions/variables, only the extern linkage ones for correctness?</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Sun, Jul 16, 2017 at 8:26 PM David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Looks good - does this support available_externally definitions of strong external linkage functions in the users of a module? (is that tested?) Should it?<br><br>Also should we consider having two flags for modular codegen - one for correctness (external function definitions), one for linkage size optimization (inline functions). Given the current data on optimized builds with inline functions (that it hurts object size to emit weak+available_externally definitions rather than linkonce_odr because so many definitions are optimized away entirely, that the bytes for the weak definition are wasted/unnecessary) - or at least something to keep in mind/run numbers on in the future for more generic codebases than Google's protobuf-heavy (& only protobuf modularized) code.</div><br><div class="gmail_quote"><div dir="ltr">On Wed, Jul 5, 2017 at 5:30 PM Richard Smith via cfe-commits <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Author: rsmith<br>
Date: Wed Jul 5 17:30:00 2017<br>
New Revision: 307232<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=307232&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=307232&view=rev</a><br>
Log:<br>
[modules ts] Do not emit strong function definitions from the module interface unit in every user.<br>
<br>
Added:<br>
cfe/trunk/test/CXX/modules-ts/basic/basic.def.odr/<br>
cfe/trunk/test/CXX/modules-ts/basic/basic.def.odr/p4/<br>
cfe/trunk/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp<br>
cfe/trunk/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cppm<br>
cfe/trunk/test/CXX/modules-ts/basic/basic.def.odr/p4/user.cpp<br>
Modified:<br>
cfe/trunk/lib/Serialization/ASTWriterDecl.cpp<br>
<br>
Modified: cfe/trunk/lib/Serialization/ASTWriterDecl.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriterDecl.cpp?rev=307232&r1=307231&r2=307232&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Serialization/ASTWriterDecl.cpp?rev=307232&r1=307231&r2=307232&view=diff</a><br>
==============================================================================<br>
--- cfe/trunk/lib/Serialization/ASTWriterDecl.cpp (original)<br>
+++ cfe/trunk/lib/Serialization/ASTWriterDecl.cpp Wed Jul 5 17:30:00 2017<br>
@@ -2233,8 +2233,18 @@ void ASTRecordWriter::AddFunctionDefinit<br>
Writer->ClearSwitchCaseIDs();<br>
<br>
assert(FD->doesThisDeclarationHaveABody());<br>
- bool ModulesCodegen = Writer->Context->getLangOpts().ModulesCodegen &&<br>
- Writer->WritingModule && !FD->isDependentContext();<br>
+ bool ModulesCodegen = false;<br>
+ if (Writer->WritingModule && !FD->isDependentContext()) {<br>
+ // Under -fmodules-codegen, codegen is performed for all defined functions.<br>
+ // When building a C++ Modules TS module interface unit, a strong definition<br>
+ // in the module interface is provided by the compilation of that module<br>
+ // interface unit, not by its users. (Inline functions are still emitted<br>
+ // in module users.)<br>
+ ModulesCodegen =<br>
+ Writer->Context->getLangOpts().ModulesCodegen ||<br>
+ (Writer->WritingModule->Kind == Module::ModuleInterfaceUnit &&<br>
+ Writer->Context->GetGVALinkageForFunction(FD) == GVA_StrongExternal);<br>
+ }<br>
Record->push_back(ModulesCodegen);<br>
if (ModulesCodegen)<br>
Writer->ModularCodegenDecls.push_back(Writer->GetDeclRef(FD));<br>
<br>
Added: cfe/trunk/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp?rev=307232&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp?rev=307232&view=auto</a><br>
==============================================================================<br>
--- cfe/trunk/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp (added)<br>
+++ cfe/trunk/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cpp Wed Jul 5 17:30:00 2017<br>
@@ -0,0 +1,23 @@<br>
+// RUN: %clang_cc1 -fmodules-ts %S/module.cppm -triple %itanium_abi_triple -emit-module-interface -o %t<br>
+// RUN: %clang_cc1 -fmodules-ts %s -triple %itanium_abi_triple -fmodule-file=%t -emit-llvm -o - | FileCheck %s --implicit-check-not=unused --implicit-check-not=global_module<br>
+<br>
+module Module;<br>
+<br>
+void use() {<br>
+ // CHECK: define linkonce_odr {{.*}}@_Z20used_inline_exportedv<br>
+ used_inline_exported();<br>
+ // CHECK: declare {{.*}}@_Z18noninline_exportedv<br>
+ noninline_exported();<br>
+<br>
+ // FIXME: This symbol should not be visible here.<br>
+ // CHECK: define internal {{.*}}@_ZL26used_static_module_linkagev<br>
+ used_static_module_linkage();<br>
+<br>
+ // FIXME: The module name should be mangled into the name of this function.<br>
+ // CHECK: define linkonce_odr {{.*}}@_Z26used_inline_module_linkagev<br>
+ used_inline_module_linkage();<br>
+<br>
+ // FIXME: The module name should be mangled into the name of this function.<br>
+ // CHECK: declare {{.*}}@_Z24noninline_module_linkagev<br>
+ noninline_module_linkage();<br>
+}<br>
<br>
Added: cfe/trunk/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cppm<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cppm?rev=307232&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cppm?rev=307232&view=auto</a><br>
==============================================================================<br>
--- cfe/trunk/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cppm (added)<br>
+++ cfe/trunk/test/CXX/modules-ts/basic/basic.def.odr/p4/module.cppm Wed Jul 5 17:30:00 2017<br>
@@ -0,0 +1,54 @@<br>
+// RUN: %clang_cc1 -fmodules-ts %s -triple %itanium_abi_triple -emit-llvm -o - | FileCheck %s --implicit-check-not=unused<br>
+<br>
+static void unused_static_global_module() {}<br>
+static void used_static_global_module() {}<br>
+inline void unused_inline_global_module() {}<br>
+inline void used_inline_global_module() {}<br>
+// CHECK: define void {{.*}}@_Z23noninline_global_modulev<br>
+void noninline_global_module() {<br>
+ // FIXME: This should be promoted to module linkage and given a<br>
+ // module-mangled name, if it's called from an inline function within<br>
+ // the module interface.<br>
+ // (We should try to avoid this when it's not reachable from outside<br>
+ // the module interface unit.)<br>
+ // CHECK: define internal {{.*}}@_ZL25used_static_global_modulev<br>
+ used_static_global_module();<br>
+ // CHECK: define linkonce_odr {{.*}}@_Z25used_inline_global_modulev<br>
+ used_inline_global_module();<br>
+}<br>
+<br>
+export module Module;<br>
+<br>
+export {<br>
+ // FIXME: These should be ill-formed: you can't export an internal linkage<br>
+ // symbol, per [dcl.module.interface]p2.<br>
+ static void unused_static_exported() {}<br>
+ static void used_static_exported() {}<br>
+<br>
+ inline void unused_inline_exported() {}<br>
+ inline void used_inline_exported() {}<br>
+ // CHECK: define void {{.*}}@_Z18noninline_exportedv<br>
+ void noninline_exported() {<br>
+ // CHECK: define internal {{.*}}@_ZL20used_static_exportedv<br>
+ used_static_exported();<br>
+ // CHECK: define linkonce_odr {{.*}}@_Z20used_inline_exportedv<br>
+ used_inline_exported();<br>
+ }<br>
+}<br>
+<br>
+static void unused_static_module_linkage() {}<br>
+static void used_static_module_linkage() {}<br>
+inline void unused_inline_module_linkage() {}<br>
+inline void used_inline_module_linkage() {}<br>
+// FIXME: The module name should be mangled into the name of this function.<br>
+// CHECK: define void {{.*}}@_Z24noninline_module_linkagev<br>
+void noninline_module_linkage() {<br>
+ // FIXME: This should be promoted to module linkage and given a<br>
+ // module-mangled name, if it's called from an inline function within<br>
+ // the module interface.<br>
+ // CHECK: define internal {{.*}}@_ZL26used_static_module_linkagev<br>
+ used_static_module_linkage();<br>
+ // FIXME: The module name should be mangled into the name of this function.<br>
+ // CHECK: define linkonce_odr {{.*}}@_Z26used_inline_module_linkagev<br>
+ used_inline_module_linkage();<br>
+}<br>
<br>
Added: cfe/trunk/test/CXX/modules-ts/basic/basic.def.odr/p4/user.cpp<br>
URL: <a href="http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/modules-ts/basic/basic.def.odr/p4/user.cpp?rev=307232&view=auto" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/cfe/trunk/test/CXX/modules-ts/basic/basic.def.odr/p4/user.cpp?rev=307232&view=auto</a><br>
==============================================================================<br>
--- cfe/trunk/test/CXX/modules-ts/basic/basic.def.odr/p4/user.cpp (added)<br>
+++ cfe/trunk/test/CXX/modules-ts/basic/basic.def.odr/p4/user.cpp Wed Jul 5 17:30:00 2017<br>
@@ -0,0 +1,13 @@<br>
+// RUN: %clang_cc1 -fmodules-ts %S/module.cppm -triple %itanium_abi_triple -emit-module-interface -o %t<br>
+// RUN: %clang_cc1 -fmodules-ts %s -triple %itanium_abi_triple -fmodule-file=%t -emit-llvm -o - | FileCheck %s --implicit-check-not=unused --implicit-check-not=global_module<br>
+<br>
+import Module;<br>
+<br>
+void use() {<br>
+ // CHECK: define linkonce_odr {{.*}}@_Z20used_inline_exportedv<br>
+ used_inline_exported();<br>
+ // CHECK: declare {{.*}}@_Z18noninline_exportedv<br>
+ noninline_exported();<br>
+<br>
+ // Module-linkage declarations are not visible here.<br>
+}<br>
<br>
<br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></blockquote></div>
</blockquote></div>
</blockquote></div>