[PATCH] D82745: [ThinLto] Fix Ifunc symbol usage

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Thanks for doing this and sorry for the slow reply! Some comments and suggested fixes below.



================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:452
 
-  void setAliasee(ValueInfo &AliaseeVI, GlobalValueSummary *Aliasee) {
-    AliaseeValueInfo = AliaseeVI;
-    AliaseeSummary = Aliasee;
+  void setSummary(ValueInfo &ValueInfo, GlobalValueSummary *Summary) {
+    IndirectValueInfo = ValueInfo;
----------------
Changing Aliasee to Summary here and below is a little confusing, since it is ambiguous what summary you are talking about (the current one, the indirect symbol?). Suggest changing Summary here and in below interfaces to IndirectSymbol. It's a little longer but captures the meaning better (and is equivalent to similar interface naming on the GlobalIndirectSymbol itself).


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:359
+        // We want to record the call edge to the aliasee or resolver in that
+        // case. Eventually an alias summary will be created to associate the
+        // alias and aliasee or ifunc and its resolver.
----------------
s/an alias summary/an indirect value summary/


================
Comment at: llvm/lib/AsmParser/LLParser.cpp:8431
 
 /// AliasSummary
 ///   ::= 'alias' ':' '(' 'module' ':' ModuleReference ',' GVFlags ','
----------------
Needs equivalent handling for the new IfuncSummary. To test this handling, add an ifunc summary case to llvm/test/Assembler/thinlto-summary.ll. Actually, add two cases, equivalent to the following two alias cases:

; Make this one an alias with a forward reference to aliasee.
^5 = gv: (guid: 4, summaries: (alias: (module: ^0, flags: (linkage: private, notEligibleToImport: 0, live: 0, dsoLocal: 1), aliasee: ^14)))

and

; Alias summary with backwards reference to aliasee.
^18 = gv: (guid: 17, summaries: (alias: (module: ^0, flags: (linkage: external, notEligibleToImport: 0, live: 0, dsoLocal: 1), aliasee: ^14)))


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:460
+            Callback({AS->getSummaryGUID(), &AS->getSummary()}, true);
+          if (auto *IF = dyn_cast<IfuncSummary>(Summary.getSecond()))
+            Callback({IF->getSummaryGUID(), &IF->getSummary()}, true);
----------------
Could this just be merged into the above alias handling by casting to GlobalIndirectSymbol?


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:4136
+      // global to be loaded first.
+      Ifuncs.push_back(IF);
+      return;
----------------
I don't see the associated post-pass for the Ifuncs. See where the Aliases vector is walked down around line 4251.


================
Comment at: llvm/lib/IR/Globals.cpp:169
     return "";
+  } else if (auto *GA = dyn_cast<GlobalIFunc>(this)) {
+    if (const GlobalObject *GO = GA->getBaseObject())
----------------
Could this just be merged into the above alias handling by casting to GlobalIndirectSymbol?


================
Comment at: llvm/lib/IR/ModuleSummaryIndex.cpp:592
 
       if (auto *AS = dyn_cast_or_null<AliasSummary>(SummaryIt.second)) {
+        Draw(SummaryIt.first, AS->getSummaryGUID(), -4);
----------------
Cast to GlobalIndirectSymbolSummary so this covers both cases?


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:536
   for (auto &GVSummary : DefinedGVSummaries) {
-#ifndef NDEBUG
     // FIXME: Change the GVSummaryMapTy to hold ValueInfo instead of GUID
----------------
This is an expensive hash table lookup (related to the below FIXME), which is why we only want to do this when necessary (currently under debug compiler for the debug message below). Suggestion later on why you don't need this.


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:549
       continue;
+    if ((!VI.getSummaryList().empty()) &&
+        (isa<IfuncSummary>(VI.getSummaryList()[0].get())))
----------------
We already have a representative summary in GVSummary.second. However, I don't think you even want to skip ifuncs here. This is just deciding what to import for any functions defined in the current module. What you really want to do is to skip trying to import the ifunc base object on any references made to an ifunc in the function. To do that, you'll want to modify within selectCallee, which is called by computeImportForFunction for everything it calls. Specifically, the code which is around line 215:

auto *Summary = cast<FunctionSummary>(GVSummary->getBaseObject());

You will want to check before calling getBaseObject() if GVSummary is an IfuncSummary and return false from the lambda function so that we don't try to import the ifunc resolver.

After looking at your tests, I'm surprised the new ifunc_import test is working. I.e. this loop should see function main(), which isn't an ifunc, add it, and then compute imports based on all of its calls, which include all of the ifuncs. Ah - I see what is going on, you are checking that the ifuncs themselves are still declarations. But we would import the base object, which is the resolver. That test should probably check whether we are importing the resolver functions or not.

Actually, it shouldn't really be a problem to import them, since we haven't imported the defs of the ifuncs that resolve to them, they should get thrown away since there won't be any references to them after importing. However, it's just wasteful work that can be avoided by the change I suggest above.



================
Comment at: llvm/test/Bitcode/thinlto-ifunc.ll:4
+; RUN: llvm-bcanalyzer -dump %t.o | FileCheck %s
+; RUN: opt -module-summary %s -o %t2.o
+; RUN: llvm-dis -o - %t2.o | FileCheck %s --check-prefix=DIS
----------------
You don't need to redo the opt invocation, they are identical. Just use %t.o in the below llvm-dis.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82745/new/

https://reviews.llvm.org/D82745