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

Mehdi AMINI via llvm-commits llvm-commits at lists.llvm.org
Mon May 23 14:07:48 PDT 2016

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.`


More information about the llvm-commits mailing list