[PATCH] D158961: [ThinLTO] Mark callers of local ifunc not eligible for import

Mon Aug 28 15:16:41 PDT 2023

MaskRay marked an inline comment as done.
MaskRay added a comment.

In D158961#4620848 <https://reviews.llvm.org/D158961#4620848>, @yota9 wrote:

> Hello! Just wanna to add that if you have interest in my patch - fill free to commander the change, modify and submit it as your own patch, I have absolutely no objections on it. Although ifuncs are quite rare so I think not importing such functions is good-enough solution. Thanks! :)

Thank you for the offer! My current thought is that we probably don't want to analyze `GlobalIFunc`. Unlike GlobalAlias whose aliasee is easy to analyze, a `GlobalIFunc` may have multiple implementations and the implementations are selected by a resolver.
It may be too much for the compiler to analyze... We have done a few special cases like D129009 <https://reviews.llvm.org/D129009> and D150262 <https://reviews.llvm.org/D150262>. With this patch, it seems that we are good enough for known cases...

My concern with a more powerful `GlobalIndirectSummary` is when we do future optimizations on `GlobalAlias`, we'd be tempted to think whether that works with `GlobalIFunc`. Since ifunc semantics are so special, there will be ongoing cognitive complexity...

================
Comment at: llvm/test/ThinLTO/X86/ifunc-import.ll:13
+
+;; The ifunc implementations are internal in A, so they cannot be referenced by B.
+;; Our implementation actually ensures that the ifunc resolver along with its
----------------
tejohnson wrote:
> This seems to be specifically about @foo.ifunc, not @foo.ifunc2 which is not internal and for which we can import a reference. 
Thanks for noticing the inaccurate comment. Fixed.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158961