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

[Hongtao Yu via Phabricator via llvm-commits]

hoyFB added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:531
+    auto isDeclaration = [](const Function *F) {
+      return !F || F->isDeclaration();
+    };
----------------
wenlei wrote:
> 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.
Yeah, that's true. The code is only run in ThinLTOPreLink, so checking `isDeclaration` is fine here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79379