[PATCH] D20290: [ThinLTO] Change ODR resolution and internalization to be index-based

[Mehdi AMINI via llvm-commits]

joker.eph accepted this revision.
joker.eph added a comment.
This revision is now accepted and ready to land.

LGTM, with a few minor comments.
(llvm-tblgen is linked bit-to-bit identical with this patch, still building clang for safety)


================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:337
@@ +336,3 @@
+    } else {
+      if (GlobalValue::isExternalLinkage(S->linkage()))
+        S->setLinkage(GlobalValue::InternalLinkage);
----------------
Minor preference for `else if`

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:339
@@ +338,3 @@
+        S->setLinkage(GlobalValue::InternalLinkage);
+    }
+  }
----------------
Can we internalize other than ExternalLinkage?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:845
@@ -791,3 +844,3 @@
 void ThinLTOCodeGenerator::internalize(Module &TheModule,
                                        ModuleSummaryIndex &Index) {
   initTMBuilder(TMBuilder, Triple(TheModule.getTargetTriple()));
----------------
The fact that the index is now modified as a side effect should be mentioned in the function description. I'm not sure: can it be reused for internalizing another module?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:874
@@ +873,3 @@
+  // supply anything to preserve.
+  if (!ExportList.empty() || !GUIDPreservedSymbols.empty()) {
+    thinLTOInternalizeModule(TheModule,
----------------
Should we move this check a few lines earlier and early return?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:962
@@ +961,3 @@
+  // producing a hash for the cache entry.
+  StringMap<std::map<GlobalValue::GUID, GlobalValue::LinkageTypes>> ResolvedODR;
+
----------------
Add in the comment that the *only* reason to have this map is to support the cache.
And I'd also add `FIXME: we should be able to compute the caching hash for the entry based on the index, and nuke this map.`



http://reviews.llvm.org/D20290