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

[Wei Mi via Phabricator via llvm-commits]

wmi added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:531
+    auto *F = M->getFunction(getFuncName());
+    if (!F || !F->getSubprogram()) {
+      // Add to the import list only when it's defined out of module.
----------------
wmi wrote:
> wenlei wrote:
> > I think we need `F->isDeclaration()` here, `getSubprogram()` tells us whether debug metadata (`!dbg`) is available for this function.
> Indeed when F->isDeclaration() is true, getSubprogram could also be true. That will potentially miss importing.  
> 
> I am not very sure about the original intention of using getSubprogram. I am thinking about the case when a comdat function doesn't have debug information in current module, is it possible to import another version with debug information from other module?  Need Teresa's opinion on it. I will ask her about it.  
I checked with Teresa. Currently if there has been a function definition in the module, no import of the same function from other module will be done, no matter how the debug information looks like. So using getSubProgram here is not intended to import comdat function with debug information. I agree we should change it to isDeclaration.    


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:540-541
         if (TS.getValue() > Threshold) {
           const Function *Callee = M->getFunction(getFuncName(TS.getKey()));
-          if (!Callee || !Callee->getSubprogram())
+          if (!Callee || Callee->isDeclaration())
             S.insert(getGUID(TS.getKey()));
----------------
We can create a lambda so the code snippet here and above can share it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79379