[PATCH] D18346: ThinLTO: special handling for LinkOnce functions

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

> Always import LinkOnce when a caller is imported. This ensure that every module with a call to a LinkOnce has the definition and will be able to emit it if it emits the call.


Note that this first approach already happens when using the ModuleLinker (as the function import pass does): referenced linkonce are added to the ValuesToLink via a callback. Which is required for correctness if we end up with any linkonce. But this is an optimization to avoid needing to force import those referenced linkonce.

And this patch also reduces the amount of weak functions kept through codegen (summary should probably reflect that it is modifying weak as well).


================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:121
@@ +120,3 @@
+
+static void FixupLinkOnceODR(GlobalValue &GV, const ModuleSummaryIndex &Index,
+                             StringRef ModulePath) {
----------------
Routine is handling more than just linkonce but also weak. Name of this routine and the following one needs to change to reflect broader scope. How about something like "ResolveWeakAndLinkOnce"?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:161
@@ +160,3 @@
+  }
+  case GlobalValue::LinkOnceODRLinkage: {
+    auto *GVInfo = getGVInfo(GV);
----------------
This block is almost identical to above, fold the cases together and just select the right Weak linkage?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:185
@@ +184,3 @@
+  }
+  case GlobalValue::WeakODRLinkage: {
+    auto *GVInfo = getGVInfo(GV);
----------------
Ditto here, this block can be folded with the above, it is identical.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:198
@@ +197,3 @@
+
+/// LinkOnceODR needs to be turned to weak ODR for correctness.
+///
----------------
Not really for correctness, as the IRMover will pull in any referenced linkonce defs. This is an optimization to reduce required importing though.

Also, they aren't all being converted to weak ODR, only one and the others are available_externally.


================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:208
@@ +207,3 @@
+  }
+  // Fixme: alias?
+}
----------------
Yes I think so

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:316
@@ -201,1 +315,3 @@
 
+    // Fixup the LinkOnceODR
+    // !! Need to be taken in account when handling the cache !!
----------------
More explanatory comment needed.

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:317
@@ +316,3 @@
+    // Fixup the LinkOnceODR
+    // !! Need to be taken in account when handling the cache !!
+    FixupLinkOnceODR(TheModule, Index);
----------------
Specify in what way.


http://reviews.llvm.org/D18346