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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

Sorry for the delayed review!

Looking at the code here, all of the handling for ifuncs is essentially now combined with or a duplicate of the equivalent handling for alias summary, I'm more convinced that the summary hierarchy should be changed to mirror the symbol hierarchy. i.e. a GlobalIndirectSummary with derived AliasSummary and IfuncSummary. Then the vast majority of the code modified here could be simply generalized to a single handling of GlobalIndirectSummary and its getBaseObject. BTW, there is still some missing handling for importing, see specifics further below.

However - before you try this I would recommend creating a more robust set of tests. The only test added here now simply tests the serialization/deserialization of the new summary to bitcode and llvm assembly.  Tests are needed that actually invoke the thin link, e.g. via either llvm-lto or llvm-lto2, see tests in llvm/test/ThinLTO/X86/ for examples. The alias_import.ll and alias_resolution.ll tests there are good to model new ifunc tests on.

One of the tests should check the case where one module references an external symbol defined as an ifunc in the other module, to test the handling where we try to import it. AFAICT, the code in FunctionImport.cpp in computeImportForFunction with the comment '"Resolve" the summary', which gets the base object, will attempt to import an ifunc resolver. However, the handling in the backend where we actually do the importing (see importFunctions in the same file) will completely ignore any ifunc that we decided to import. We should either (conservatively for now) prevent importing of ifunc during the thin link (computeImportForFunction), or handle ifunc appropriately in importFunctions (not sure what the appropriate handling is - for alias we import as a copy of the aliasee, does that even make sense for ifunc? If not, probably the conservative handling is best).



================
Comment at: llvm/lib/IR/Verifier.cpp:383
 
+    for (const GlobalIFunc &I : M.ifuncs())
+      visitGlobalIfunc(I);
----------------
The changes in this file seem unrelated to ThinLTO. Can you split into another patch (with test)?


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

https://reviews.llvm.org/D82745