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

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

See question below about linkage types for internalization, will hold off on submit for now.


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

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:339
@@ +338,3 @@
+        S->setLinkage(GlobalValue::InternalLinkage);
+    }
+  }
----------------
joker.eph wrote:
> Can we internalize other than ExternalLinkage?
Good question. Looking at the rest of the linkage types they are either available externally (skipped by internalizeModule), various types of weak (including linkonce), and other special types such as common and appending.

I'm not sure about the weak/linkonce types - I don't see anything in internalizeModule or elsewhere in HEAD that prevents internalizing these symbols, but I would have thought that internalizing them could be problematic since they have weak linkage and some can be overridden by external definitions (isInterposableLinkage). I assume that with ld64 any that need to be preserved because they are overridden are added to the preserved set? In gold we know which one is prevailing (which is used to guide current internalization decisions in gold-plugin) and can use that info to guide the internalization via the callback when I call this from there.

I removed the "if (GlobalValue::isExternalLinkage(S->linkage()))" guard and the regression tests run fine - do you want to try your bootstrap with that change?

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:845
@@ -791,3 +844,3 @@
 void ThinLTOCodeGenerator::internalize(Module &TheModule,
                                        ModuleSummaryIndex &Index) {
   initTMBuilder(TMBuilder, Triple(TheModule.getTargetTriple()));
----------------
joker.eph wrote:
> 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?
Done here (and in the description of promote() which modifies the index for resolved weak symbols). 

It would be reusable for another module - since the analysis is global on the index the decisions should be the same for all modules. The index updates would happen the first time through here.

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

================
Comment at: lib/LTO/ThinLTOCodeGenerator.cpp:962
@@ +961,3 @@
+  // producing a hash for the cache entry.
+  StringMap<std::map<GlobalValue::GUID, GlobalValue::LinkageTypes>> ResolvedODR;
+
----------------
joker.eph wrote:
> 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.`
> 
Done


http://reviews.llvm.org/D20290