[PATCH] D146770: [Propeller] Match debug info filenames from profiles to distinguish internal linkage functions with the same names.

[Rahman Lavaee via Phabricator via llvm-commits]

rahmanl marked 5 inline comments as done.
rahmanl added a comment.

Thanks for the quick feedback @tmsriram



================
Comment at: llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp:46
+              ? std::pair(true, R->second)
+              : std::pair(false, SmallVector<BBClusterInfo>{}));
 }
----------------
tmsriram wrote:
> Isn't this a refactoring change that should go into a separate patch?
Thanks. Checked in a small patch: https://github.com/llvm/llvm-project/commit/ef8dedc79ce02710ffba6a3005e05672702f555f


================
Comment at: llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp:94
       if (FI == ProgramBBClusterInfo.end())
-        return invalidProfileError(
-            "Cluster list does not follow a function name specifier.");
+        continue;
       SmallVector<StringRef, 4> BBIDs;
----------------
tmsriram wrote:
> How is this valid now?
I silenced this error handling to allow the condition to imply that the profile must be skipped.


================
Comment at: llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp:131
+      bool FunctionFound = any_of(Aliases, [&](StringRef Alias) {
+        auto It = FunctionNameToDIFilename.find(Alias);
+        return It != FunctionNameToDIFilename.end()
----------------
tmsriram wrote:
> Here somehow, can you get the linkage type of Alias and discard cases where it is a non internal linkage function?  I haven't thought of how to do it yet.
Yes. This is doable in the doInitialization call. However, I am not sure if we should discard these cases. My point is "if the debug-info-filename is specified, we should require that it is matched. If it is omitted, then it will be matched with anything".


================
Comment at: llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp:134
+                   ? (DIFilename.empty() || It->second.empty() ||
+                      It->second.equals(DIFilename))
+                   : false;
----------------
tmsriram wrote:
> First of all, you shouldnt be checking DIFilename.empty() here, that case must be dismissed much earlier above. If DIFilename is empty, simply don't check anything as this is equal to doing the default and do it immediately after getting DIFilename above.
> 
> The condition must also look like this:
> 
> ```
>   return It != FunctionNameToDIFilename.end() ? It->second.equals(DIFilename) : false;
> ```
> 
> If It->second is empty and we have specified a module, we should conclude false, WDYT?
This is a bit subtle. If debug-info-filename specifier is empty, we should still check that the function can be found in the module. This won't  change the final result, but it makes the internal logic consistent with the case when debug-info-filename is specified (where both function name and debug-info filename are checked).



================
Comment at: llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp:166
+    SmallString<128> DIFilename;
+    if (F.isDeclaration()) continue;
+    auto *Subprogram = F.getSubprogram();
----------------
tmsriram wrote:
> Not a great optimization but only internal linkage function definitions  here really matter right? 
> 
> I am not sure but it appears to me that you are storing every defined function name in the map to make the other check work.  That is not wrong but you may be able to do something better.
> 
> Here is an idea:
> Store only internal linkage functions in the map.  If you cannot find it in the map, then clearly it is not an internal linkage function name corresponding to this module and its cluster info can be safely discarded.  However, there is an argument that global linkage functions can also have their modules specified but should be made beyond the scope of this feature.  WDYT?
```
If you cannot find it in the map, then clearly it is not an internal linkage function name corresponding to this module and its cluster info can be safely discarded.
```

But we are not supposed to discard the profiles for global linkage functions. right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146770