[PATCH] D53254: [Merge SImilar Function ThinLTO 5/n] Set up similar function to be imported

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: lib/Transforms/IPO/FunctionImport.cpp:519
+            auto FS = cast<FunctionSummary>(S);
+            // TODO: Import declarations of global variables as well.
+            if (FS->refs().size())
----------------
tejohnson wrote:
> hiraditya wrote:
> > tejohnson wrote:
> > > Why this constraint? Note that any global variables being referenced by an imported function will automatically have their declarations imported too.
> > I'll remove this comment. Does not seem to be relevant.
> It isn't just the comment, but also the corresponding check just below that the refs() be empty.
Note though that in its place you need to ensure that the function is eligible to import (notEligibleToImport() on the function summary), and you should also skip anything that was found to be globally unreachable (isGlobalValueLive() on the index). Not sure about checking whether it has interposable linkage. See some of the checks being done in selectCallee() in this file. Some of those are profitability (related to whether we can inline the imported function), but others are correctness checks. I.e. if it is notEligibleToImport then it might be referencing functions/variables that cannot be promoted to global scope for some reason.


https://reviews.llvm.org/D53254