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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Yikes, I guess ifunc weren't used very extensively before if we have been dropping them on the floor with thinlto all this time!

There are some uses of alias summaries other places (e.g. in ModuleSummaryIndex.cpp for exporting to a dot file, computing the cache key in LTO.cpp) that you'll want to adjust as well. There's some other uses of AliasSummary that may need similar handling as well that I'm not sure about offhand. E.g. see the uses of AliasSummary in LTO.cpp where we decide whether we can adjust the linkage to AvailableExternally. Also see my comments on the def of GlobalValueSummary::getBaseObject and one of its uses in FunctionImport.cpp since it is not being handled there. If we do decide to try to import ifuncs and resolvers, you'll need to think about how this should work when we do the importing. For aliases, we import the aliasee as a copy (see replaceAliasWithAliasee in FunctionImport.cpp). I'm not familiar enough with ifunc to know how it should be handled when importing.

Also, since most of the handling here for ifunc summary is essentially an exact duplicate of the equivalent alias summary handling, I'm wondering if we can just generalize the AliasSummary to be a more general GlobalIndirectionSummary? Perhaps we could distinguish between the two types as needed with different SummaryKind?

Before changing the patch significantly though, I think it would be good to decide how these should be handled for other types of ThinLTO optimizations as mentioned above (e.g. importing and weak symbol resolution). I'm fine with treating them conservatively (e.g. not importing), but right now as noted above I think we might get runtime failures when these are seen during importing analysis.



================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:365
       : Kind(K), Flags(Flags), RefEdgeList(std::move(Refs)) {
     assert((K != AliasKind || Refs.empty()) &&
            "Expect no references for AliasSummary");
----------------
Should be same for ifunc kind


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:530
 const inline GlobalValueSummary *GlobalValueSummary::getBaseObject() const {
   if (auto *AS = dyn_cast<AliasSummary>(this))
     return &AS->getAliasee();
----------------
Should this (and the method below) return the resolver for IfuncSummary? See my comment in FunctionImport.cpp in one of the callsites for what I think will happen currently.


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:358
+        // Use the original CalledValue, in case it was an alias or ifunc.
+        // We want to record the call edge to the alias in that case.
+        // Eventuallyan alias summary will be created to associate the alias
----------------
"aliasee or resolver"?


================
Comment at: llvm/lib/Analysis/ModuleSummaryAnalysis.cpp:359
+        // We want to record the call edge to the alias in that case.
+        // Eventuallyan alias summary will be created to associate the alias
+        // and aliasee or ifunc and its resolver.
----------------
Missing space between first 2 words.


================
Comment at: llvm/lib/Transforms/IPO/FunctionImport.cpp:215
 
         auto *Summary = cast<FunctionSummary>(GVSummary->getBaseObject());
 
----------------
With the current patch this will fail if the callee is an IfuncSummary, since getBaseObject on the GlobalValueSummary only handles alias summaries.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82745