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

[Sriraman Tallam via Phabricator via llvm-commits]

tmsriram added a comment.

Just to add to the limitations of -funique-internal-linkage-functions, even the alias attribute can cause problems.  Any attribute that takes the name of a function as a string, alias / asm, will not work with this option as it involves changing the source.  The kernel has plenty of these and hence we need a better solution.



================
Comment at: llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp:46
+              ? std::pair(true, R->second)
+              : std::pair(false, SmallVector<BBClusterInfo>{}));
 }
----------------
Isn't this a refactoring change that should go into a separate patch?


================
Comment at: llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp:59
 // !main
 // !foo
 // !!1 2
----------------
Should the comments be changed to reflect how module names can be specified?


================
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;
----------------
How is this valid now?


================
Comment at: llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp:131
+      bool FunctionFound = any_of(Aliases, [&](StringRef Alias) {
+        auto It = FunctionNameToDIFilename.find(Alias);
+        return It != FunctionNameToDIFilename.end()
----------------
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.


================
Comment at: llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp:134
+                   ? (DIFilename.empty() || It->second.empty() ||
+                      It->second.equals(DIFilename))
+                   : false;
----------------
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?


================
Comment at: llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp:149
+      if (!R.second)
+        return invalidProfileError("Duplicate profile for function: '" +
+                                   Aliases.front() + "'.");
----------------
Why would this be problem now and not earlier?  Add a comment to make it clear how duplicate function names could happen.  


================
Comment at: llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp:166
+    SmallString<128> DIFilename;
+    if (F.isDeclaration()) continue;
+    auto *Subprogram = F.getSubprogram();
----------------
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?


================
Comment at: llvm/test/CodeGen/X86/basic-block-sections-clusters-error.ll:21
+; RUN: not --crash llc < %s -O0 -mtriple=x86_64 -function-sections -basic-block-sections=%t5 2>&1 | FileCheck %s --check-prefix=CHECK-ERROR5
+; CHECK-ERROR5: LLVM ERROR: Invalid profile {{.*}} at line 2: Duplicate profile for function: 'dummy'.
 
----------------
Ah okay, that's how you get duplicate profiles.  Understood but please make sure this is documented in the code above.  Also, this case could have occurred even before where you specify multiple profiles for the same function.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146770