Make -linkonce-templates less aggressive by default & add -linkonce-t…

…emplates-aggressive (#3924) Make -linkonce-templates *not* tamper with the general template emission algorithm anymore (so on top of default non-allinst or -allinst modes), and keep those tweaks as experimental -linkonce-templates-aggressive. Compiling the druntime/Phobos unittests is only marginally slowed down compared to the more aggressive variant (~1.5% for debug, ~2.5% for release). It does show some rough 10% increase in required memory, but that's in line with non-linkonce-templates. The more aggressive variant has the advantage of skipping needsCodegen() and potentially codegen'ing less symbols. The problem is that if an instantiated symbol isn't explicitly referenced, for instance a CRT ctor, it might not be codegen'd at all.
ldc-developers · Feb 28, 2022 · 4cbaebd · 4cbaebd
1 parent f2a6fef
commit 4cbaebd
Show file tree

Hide file tree

Showing 8 changed files with 50 additions and 25 deletions.
diff --git a/dmd/dsymbolsem.d b/dmd/dsymbolsem.d
@@ -5890,8 +5890,8 @@ void templateInstanceSemantic(TemplateInstance tempinst, Scope* sc, Expressions*
         }
 
         // LDC: the tnext linked list is only used by TemplateInstance.needsCodegen(),
-        //      which is skipped with -linkonce-templates
-        if (!(IN_LLVM && global.params.linkonceTemplates))
+        //      which is skipped with -linkonce-templates-aggressive
+        if (!(IN_LLVM && global.params.linkonceTemplates == LinkonceTemplates.aggressive))
         {
             tempinst.tnext = tempinst.inst.tnext;
             tempinst.inst.tnext = tempinst;
@@ -5979,9 +5979,10 @@ void templateInstanceSemantic(TemplateInstance tempinst, Scope* sc, Expressions*
             scope v = new InstMemberWalker(tempinst.inst);
             tempinst.inst.accept(v);
 
-            if (IN_LLVM && global.params.linkonceTemplates)
+            if (IN_LLVM && global.params.linkonceTemplates == LinkonceTemplates.aggressive)
             {
-                // with -linkonce-templates, an earlier speculative or non-root instance hasn't been appended to any module yet
+                // with -linkonce-templates-aggressive, an earlier speculative or non-root instance
+                // hasn't been appended to any module yet
                 assert(tempinst.inst.memberOf is null);
                 tempinst.inst.appendToModuleMember();
             }

diff --git a/dmd/dtemplate.d b/dmd/dtemplate.d
@@ -6241,7 +6241,7 @@ extern (C++) class TemplateInstance : ScopeDsymbol
     {
 version (IN_LLVM)
 {
-        assert(!global.params.linkonceTemplates);
+        assert(global.params.linkonceTemplates != LinkonceTemplates.aggressive);
 }
 
         // minst is finalized after the 1st invocation.
@@ -7370,7 +7370,7 @@ version (IN_LLVM)
 
 version (IN_LLVM)
 {
-        if (global.params.linkonceTemplates)
+        if (global.params.linkonceTemplates == LinkonceTemplates.aggressive)
         {
             // Skip if it's not a root module.
             if (!mi || !mi.isRoot())

diff --git a/dmd/globals.d b/dmd/globals.d
@@ -116,12 +116,21 @@ enum FeatureState : byte
 }
 
 version (IN_LLVM)
+{
+enum LinkonceTemplates : byte
+{
+    no,        // non-discardable weak_odr linkage
+    yes,       // discardable linkonce_odr linkage + lazily and recursively define all referenced instantiated symbols in each object file (define-on-declare)
+    aggressive // be more aggressive wrt. speculative instantiations - don't append to module members and skip needsCodegen() culling; rely on define-on-declare.
+}
+
 enum DLLImport : byte
 {
     none,
     defaultLibsOnly, // only symbols from druntime/Phobos
     all
 }
+} // IN_LLVM
 
 /// Put command line switches in here
 extern (C++) struct Param
@@ -316,7 +325,7 @@ version (IN_LLVM)
 
     bool outputSourceLocations; // if true, output line tables.
 
-    bool linkonceTemplates; // -linkonce-templates
+    LinkonceTemplates linkonceTemplates; // -linkonce-templates
 
     // Windows-specific:
     bool dllexport;      // dllexport ~all defined symbols?

diff --git a/dmd/globals.h b/dmd/globals.h
@@ -99,6 +99,13 @@ enum class FeatureState : signed char
 };
 
 #if IN_LLVM
+enum class LinkonceTemplates : char
+{
+    no,        // non-discardable weak_odr linkage
+    yes,       // discardable linkonce_odr linkage + lazily and recursively define all referenced instantiated symbols in each object file (define-on-declare)
+    aggressive // be more aggressive wrt. speculative instantiations - don't append to module members and skip needsCodegen() culling; rely on define-on-declare.
+};
+
 enum class DLLImport : char
 {
     none,
@@ -286,7 +293,7 @@ struct Param
 
     bool outputSourceLocations; // if true, output line tables.
 
-    bool linkonceTemplates; // -linkonce-templates
+    LinkonceTemplates linkonceTemplates; // -linkonce-templates
 
     // Windows-specific:
     bool dllexport;      // dllexport ~all defined symbols?

diff --git a/driver/cl_options.cpp b/driver/cl_options.cpp
@@ -515,11 +515,16 @@ cl::opt<uint32_t, true> hashThreshold(
     "hash-threshold", cl::ZeroOrMore, cl::location(global.params.hashThreshold),
     cl::desc("Hash symbol names longer than this threshold (experimental)"));
 
-static cl::opt<bool, true> linkonceTemplates(
-    "linkonce-templates", cl::ZeroOrMore,
-    cl::location(global.params.linkonceTemplates),
-    cl::desc(
-        "Use linkonce_odr linkage for template symbols instead of weak_odr"));
+static cl::opt<LinkonceTemplates, true> linkonceTemplates(
+    cl::ZeroOrMore, cl::location(global.params.linkonceTemplates),
+    cl::values(
+        clEnumValN(LinkonceTemplates::yes, "linkonce-templates",
+                   "Use discardable linkonce_odr linkage for template symbols "
+                   "and lazily & recursively define all referenced "
+                   "instantiated symbols in each object file"),
+        clEnumValN(LinkonceTemplates::aggressive,
+                   "linkonce-templates-aggressive",
+                   "Experimental, more aggressive variant")));
 
 cl::opt<bool> disableLinkerStripDead(
     "disable-linker-strip-dead", cl::ZeroOrMore,

diff --git a/driver/main.cpp b/driver/main.cpp
@@ -449,9 +449,9 @@ void parseCommandLine(Strings &sourceFiles) {
   global.params.output_mlir = opts::output_mlir ? OUTPUTFLAGset : OUTPUTFLAGno;
   global.params.output_s = opts::output_s ? OUTPUTFLAGset : OUTPUTFLAGno;
 
-  templateLinkage = global.params.linkonceTemplates
-                        ? LLGlobalValue::LinkOnceODRLinkage
-                        : LLGlobalValue::WeakODRLinkage;
+  templateLinkage = global.params.linkonceTemplates == LinkonceTemplates::no
+                        ? LLGlobalValue::WeakODRLinkage
+                        : LLGlobalValue::LinkOnceODRLinkage;
 
   if (global.params.run || !runargs.empty()) {
     // FIXME: how to properly detect the presence of a PositionalEatsArgs

diff --git a/gen/declarations.cpp b/gen/declarations.cpp
@@ -319,13 +319,15 @@ class CodegenVisitor : public Visitor {
       return;
     }
 
-    // With -linkonce-templates, only non-speculative instances make it to
-    // module members (see `TemplateInstance.appendToModuleMember()`), and we
-    // don't need full needsCodegen() culling in that case; isDiscardable() is
-    // sufficient. Speculative ones are lazily emitted if actually referenced
+    // With -linkonce-templates-aggressive, only non-speculative instances make
+    // it to module members (see `TemplateInstance.appendToModuleMember()`), and
+    // we don't need full needsCodegen() culling in that case; isDiscardable()
+    // is sufficient. Speculative ones are lazily emitted if actually referenced
     // during codegen - per IR module.
-    if ((global.params.linkonceTemplates && decl->isDiscardable()) ||
-        (!global.params.linkonceTemplates && !decl->needsCodegen())) {
+    if ((global.params.linkonceTemplates == LinkonceTemplates::aggressive &&
+         decl->isDiscardable()) ||
+        (global.params.linkonceTemplates != LinkonceTemplates::aggressive &&
+         !decl->needsCodegen())) {
       Logger::println("Does not need codegen, skipping.");
       return;
     }

diff --git a/gen/llvmhelpers.cpp b/gen/llvmhelpers.cpp
@@ -1733,8 +1733,9 @@ static bool isDefaultLibSymbol(Dsymbol *sym) {
             (md->packages.length > 1 && md->packages.ptr[1] == Id::io)));
 }
 
-bool defineOnDeclare(Dsymbol* sym, bool) {
-  return global.params.linkonceTemplates && sym->isInstantiated();
+bool defineOnDeclare(Dsymbol *sym, bool) {
+  return global.params.linkonceTemplates != LinkonceTemplates::no &&
+         sym->isInstantiated();
 }
 
 bool dllimportDataSymbol(Dsymbol *sym) {
@@ -1750,7 +1751,7 @@ bool dllimportDataSymbol(Dsymbol *sym) {
       return !mod->isRoot(); // non-root ModuleInfo symbol
     } else if (sym->inNonRoot()) {
       return true; // not instantiated, and defined in non-root
-    } else if (!global.params.linkonceTemplates &&
+    } else if (global.params.linkonceTemplates == LinkonceTemplates::no &&
                sym->isInstantiated()) {
       return true; // instantiated but potentially culled (needsCodegen())
     } else if (auto vd = sym->isVarDeclaration()) {