[PATCH] D35875: ThinLTO: Don't import aliases of any kind (even linkonce_odr)

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Sorry for the delay. A few comments below. Regarding Mehdi's performance question - I don't expect this to have a major impact as we were only importing aliases in limited cases anyway. We'll see any performance effect in our nightly testing once this goes in, which should surface any surprise effects.



================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:135
           return false;
         if (auto *AS = dyn_cast<AliasSummary>(GVSummary)) {
+          // Aliases can't point to "available_externally".
----------------
Braces can be removed


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:138
           // FIXME: we should import alias as available_externally *function*,
           // the destination module does need to know it is an alias.
+          return false;
----------------
While you're in here, can you correct the comment: s/does needs to know/does not need to know/


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:228
     const FunctionSummary *ResolvedCalleeSummary;
     if (isa<AliasSummary>(CalleeSummary)) {
       ResolvedCalleeSummary = cast<FunctionSummary>(
----------------
This handling needs to be updated - can simply assert that CalleeSummary not an AliasSummary.


================
Comment at: lib/Transforms/Utils/FunctionImportUtils.cpp:27
   // For alias, we tie the definition to the base object. Extract it and recurse
-  if (auto *GA = dyn_cast<GlobalAlias>(SGV)) {
-    if (GA->isInterposable())
-      return false;
-    const GlobalObject *GO = GA->getBaseObject();
-    if (!GO->hasLinkOnceODRLinkage())
-      return false;
-    return FunctionImportGlobalProcessing::doImportAsDefinition(
-        GO, GlobalsToImport);
-  }
+  if (auto *GA = dyn_cast<GlobalAlias>(SGV))
+    return false;
----------------
GA is unused, and comment is stale. But I also think that this should be moved to after the below check of SGV being in GlobalsToImport, and converted to an assert (if in GlobalsToImport, better not be an alias).


================
Comment at: lib/Transforms/Utils/FunctionImportUtils.cpp:133
   case GlobalValue::ExternalLinkage:
     // External defnitions are converted to available_externally
     // definitions upon import, so that they are available for inlining
----------------
"External and linkonce definitions" (can you fix typo in "defnitions" while here)?


================
Comment at: test/ThinLTO/X86/alias_import.ll:48
 
 ; On the import side now, verify that aliases to a linkonce_odr are imported, but the weak/linkonce (we can't inline them)
 ; IMPORT-DAG:  declare void @linkonceODRfuncWeakAlias
----------------
Comment needs update.


https://reviews.llvm.org/D35875