[PATCH] D79379: Don't add function to import list if it's defined in the same module

[Wenlei He via Phabricator via llvm-commits]

wenlei accepted this revision.
wenlei added a comment.
This revision is now accepted and ready to land.

Thanks! LGTM other than the test tweak hoy pointed out. You may also want to update the summary to reflect the additional fix.

Since most of the time we actually have `!dbg` for declarations, the additional fix would enable proper profile based importing/inlining for cross-module callee's that were not previously inlined. This could have a positive performance impact (especially for multiple iterations of AutoFDO) because we were unintentionally restricted from growing cross-module inlining based on profile beyond what can be replayed by sample loader (which was from first build without profile).



================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:531
+    auto isDeclaration = [](const Function *F) {
+      return !F || F->isDeclaration();
+    };
----------------
tejohnson wrote:
> hoyFB wrote:
> > Should `isDeclarationForLinker` be checked instead? I thought when a function is imported, it has definition in the current module and it's not longer a declaration. An imported function is a definition in the module it's imported into but since it is also a `isDeclarationForLinker` (or `hasAvailableExternallyLinkage`) it won't be emitted into the current object file.
> IIUC this code is deciding what to tell ThinLTO to import. So it makes sense to see if it is an actual declaration at this point. If there is a def in the module (regardless of linkage type), when we get to the thin link we won't bother importing the callee.
> I thought when a function is imported, it has definition in the current module and it's not longer a declaration. 

This `GUID` list here is collected only in pre-link to guide importing for ThinLink, so at this point, `isDeclaration` is accurate.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79379