[ThinLTO] Remove unused (?) code from thinLTOInternalizeModule

[evgeny777]

Summary

I've found the piece of code running on ThinLTO internalization phase which seems useless to me.
Also no test seems to depend on it. I've run check-llvm after removal and it passes flawlessly.

It looks like undefined assembly symbols are first put to GlobalResolutions by lto::addModuleToGlobalRes and than
thinLTOInternalizeAndPromoteInIndex is doing the job by setting the proper linkage in ModuleSummaryIndex.

If this code is needed can someone suggest how to write unit test for it?

[evgeny777]

lib/Transforms/IPO/FunctionImport.cpp

647

The thinLTOInternalizeModule running in separate thread for each module being LTO'ed
So if we found some undefined symbol, which defined in another module what we can do now? That other
module might have already been internalized (or is being internalized at the same moment on another CPU core).

[mehdi_amini]

lib/Transforms/IPO/FunctionImport.cpp

647

The comment below says "Can't be internalized if referenced in inline asm", which isn't about cross-module reference. The weird part is that the comment above is supposed to collect "the list of symbols that are not defined in the current module", which shouldn't be possibly interfering with the symbol considered for internalization.

[mehdi_amini]

I wrote this code in April 2016 ( http://reviews.llvm.org/D19103 ), in the very first approach to internalization, which was inferred in the backend and wasn't bullet proof (especially with respect to assembly by the way).
Since then, we moved all decision in the thin-link and this code is indeed (hopefully) dead.

So LGTM! (but it may be a good idea to hear back from Teresa about this)

[evgeny777]

@tejohnson Any comments or objections?

[tejohnson]

In D40970#951028, @evgeny777 wrote:

@tejohnson Any comments or objections?

This should be fine - if we weren't marking these correctly in the summaries before the thin link we would be computing dead symbols incorrectly and have bigger problems.
For gold and lld this information is computed by and accessed via the IRSymtab. I'm not sure / can't remember how this is handled via ld64 and the legacy LTO API, but like I said, thin link dead stripping would overall not work if we didn't mark these as exported.

lib/Transforms/IPO/FunctionImport.cpp

for (auto &GV : TheModule.globals())
     updateLinkage(GV);
   for (auto &GV : TheModule.aliases())
     updateLinkage(GV);
 }
 
 /// Run internalization on \p TheModule based on symmary analysis.
 void llvm::thinLTOInternalizeModule(Module &TheModule,
                                     const GVSummaryMapTy &DefinedGlobals) {
-  // Parse inline ASM and collect the list of symbols that are not defined in
-  // the current module.
-  StringSet<> AsmUndefinedRefs;
-  ModuleSymbolTable::CollectAsmSymbols(
-      TheModule,
-      [&AsmUndefinedRefs](StringRef Name, object::BasicSymbolRef::Flags Flags) {
-        if (Flags & object::BasicSymbolRef::SF_Undefined)
-          AsmUndefinedRefs.insert(Name);
-      });
-
   // Declare a callback for the internalize pass that will ask for every
   // candidate GlobalValue if it can be internalized or not.
   auto MustPreserveGV = [&](const GlobalValue &GV) -> bool {
-    // Can't be internalized if referenced in inline asm.
-    if (AsmUndefinedRefs.count(GV.getName()))
-      return true;
-
     // Lookup the linkage recorded in the summaries during global analysis.
     auto GS = DefinedGlobals.find(GV.getGUID());
     if (GS == DefinedGlobals.end()) {
       // Must have been promoted (possibly conservatively). Find original
       // name so that we can access the correct summary and see if it can
       // be internalized again.
       // FIXME: Eventually we should control promotion instead of promoting
       // and internalizing again.