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

[rahmanl]

Summary

Basic block sections profiles are ingested based on the function name. However, conflicts may occur when internal linkage functions with the same symbol name are linked into the binary (for instance static functions defined in different modules). Currently, these functions cannot be optimized unless we use -funique-internal-linkage-names (D89617) to enforce unique symbol names.

However, we have found that -funique-internal-linkage-names does not play well with inline assembly code which refers to the symbol via its symbol name. For example, the Linux kernel does not build with this option.

This patch implements a new feature which allows differentiating profiles based on the debug info filenames associated with each function. When specified, the given path is compared against the debug info filename of the matching function and profile is ingested only when the debug info filenames match. Backward-compatibility is guaranteed as omitting the specifiers from the profile would allow them to be matched by function name only. Also specifiers can be included for a subset of functions only.

[rahmanl]

Add test.

[rahmanl]

Add test cases.

[rahmanl]

• Implement selective profile reading. Profile for each function is only retrieved if it corresponds to a function in the current module.

[rahmanl]

• Implement selective profile reading. Profile for each function is only retrieved if it corresponds to a function in the current module.

[rahmanl]

• Cleanup.

[tmsriram]

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.

llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp

52

Isn't this a refactoring change that should go into a separate patch?

63

Should the comments be changed to reflect how module names can be specified?

102

How is this valid now?

133

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.

136

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?

142

Why would this be problem now and not earlier? Add a comment to make it clear how duplicate function names could happen.

156

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?

llvm/test/CodeGen/X86/basic-block-sections-clusters-error.ll

21 ↗

(On Diff #512706)

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.

[rahmanl]

Thanks for the quick feedback @tmsriram

llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp

52

Thanks. Checked in a small patch: https://github.com/llvm/llvm-project/commit/ef8dedc79ce02710ffba6a3005e05672702f555f

102

I silenced this error handling to allow the condition to imply that the profile must be skipped.

133

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".

136

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).

156

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?

[rahmanl]

• address comments.

[rahmanl]

• Extract filename via DICompileUnits. Match on filename attribute only.

[shenhan]

Thanks for implementing this.

llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp

131

nit: backslash can be dropped in "Unknown string found: \'".

144–145

Do you think we shall make this more tolerant? Instead of returning a invalidProfileError, just drop the profile for this specific function?

154

Could you spell out the full type?

162

I'll apply this to create_llvm_prof too.

168–169

This mean there are no function definitions in the module? Is it an error?
(If it is a real error, we can make it more meaningful, like "module does not contain function defintions, etc...")

[rahmanl]

• Address reviewer comments.

llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp

144–145

I suggest we have a hard failure in such cases. Silently dropping such cases may lead to uncaught performance drops. However, avoiding duplicates is something that the profile processing tool can easily do.

168–169

Removed. It was debug printing.

[rahmanl]

• rebase.

[shenhan]

Locally tested the patch.

LGTM

llvm/include/llvm/CodeGen/BasicBlockSectionsProfileReader.h

 #include "llvm/Support/Error.h"
 #include "llvm/Support/LineIterator.h"
 #include "llvm/Support/MemoryBuffer.h"
 
 using namespace llvm;
 
 namespace llvm {
 
+struct FunctionID {
+  StringRef DIFileName;
+  SmallVector<StringRef> FunctionNames;
+};
+
 // The cluster information for a machine basic block.
 struct BBClusterInfo {
   // Unique ID for this basic block.
   unsigned BBID;
   // Cluster ID this basic block belongs to.
   unsigned ClusterID;
   // Position of basic block within the cluster.
   unsigned PositionInCluster;
 };
 
-using ProgramBBClusterInfoMapTy = StringMap<SmallVector<BBClusterInfo>>;
+struct FunctionClusterInfo {
+  FunctionID FuncID;
+  SmallVector<BBClusterInfo> Clusters;
+};
 
-class BasicBlockSectionsProfileReader : public ImmutablePass {
+class BasicBlockSectionsProfileReader : public ModulePass {
 public:
   static char ID;
 
   BasicBlockSectionsProfileReader(const MemoryBuffer *Buf)
       : ImmutablePass(ID), MBuf(Buf) {
     initializeBasicBlockSectionsProfileReaderPass(
         *PassRegistry::getPassRegistry());
   };
 
   BasicBlockSectionsProfileReader() : ImmutablePass(ID) {
     initializeBasicBlockSectionsProfileReaderPass(
         *PassRegistry::getPassRegistry());
   }
 
   StringRef getPassName() const override {
     return "Basic Block Sections Profile Reader";
   }
 
   // Returns true if basic block sections profile exist for function \p
   // FuncName.
-  bool isFunctionHot(StringRef FuncName) const;
+  bool isFunctionHot(StringRef ModuleName, StringRef FuncName) const;
 
   // Returns a pair with first element representing whether basic block sections
   // profile exist for the function \p FuncName, and the second element
   // representing the basic block sections profile (cluster info) for this
   // function. If the first element is true and the second element is empty, it
   // means unique basic block sections are desired for all basic blocks of the
   // function.
   std::pair<bool, SmallVector<BBClusterInfo>>
-  getBBClusterInfoForFunction(StringRef FuncName) const;
+  getBBClusterInfoForFunction(FunctionID FuncID) const;
 
   /// Read profiles of basic blocks if available here.
   void initializePass() override;
 
-private:
-  StringRef getAliasName(StringRef FuncName) const {
-    auto R = FuncAliasMap.find(FuncName);
-    return R == FuncAliasMap.end() ? FuncName : R->second;
+  bool runOnModule(Module &M) override {
+    for(
+    return true;
   }
 
+private:
   // This contains the basic-block-sections profile.
   const MemoryBuffer *MBuf = nullptr;
 
   // This encapsulates the BB cluster information for the whole program.
   //
   // For every function name, it contains the cluster information for (all or
   // some of) its basic blocks. The cluster information for every basic block
   // includes its cluster ID along with the position of the basic block in that
   // cluster.
-  ProgramBBClusterInfoMapTy ProgramBBClusterInfo;
+  std::vector<FunctionClusterInfo> ProgramBBClusterInfo;
 
   // Some functions have alias names. We use this map to find the main alias
   // name for which we have mapping in ProgramBBClusterInfo.
-  StringMap<StringRef> FuncAliasMap;
+  DenseMap<FunctionID, int> FunctionProfileIndexMap.
 };
 
 // Creates a BasicBlockSectionsProfileReader pass to parse the basic block
 // sections profile. \p Buf is a memory buffer that contains the list of
 // functions and basic block ids to selectively enable basic block sections.
 ImmutablePass *
 createBasicBlockSectionsProfileReaderPass(const MemoryBuffer *Buf);
 
 } // namespace llvm
 #endif // LLVM_CODEGEN_BASICBLOCKSECTIONSPROFILEREADER_H

llvm/lib/CodeGen/BasicBlockSections.cpp

 #include "llvm/ADT/StringRef.h"
 #include "llvm/CodeGen/BasicBlockSectionUtils.h"
 #include "llvm/CodeGen/BasicBlockSectionsProfileReader.h"
 #include "llvm/CodeGen/MachineFunction.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/CodeGen/TargetInstrInfo.h"
 #include "llvm/InitializePasses.h"
+#include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/Target/TargetMachine.h"
 #include <optional>
 
 using namespace llvm;
 
 // Placing the cold clusters in a separate section mitigates against poor
 // profiles and allows optimizations such as hugepage mapping to be applied at a
 // section granularity. Defaults to ".text.split." which is recognized by lld

 // This function provides the BBCluster information associated with a function.
 // Returns true if a valid association exists and false otherwise.
 bool getBBClusterInfoForFunction(
     const MachineFunction &MF,
     BasicBlockSectionsProfileReader *BBSectionsProfileReader,
     DenseMap<unsigned, BBClusterInfo> &V) {
 
   // Find the assoicated cluster information.
+  std::string ModuleName = "";
+  auto *Subprogram = MF.getFunction().getSubprogram();
+  if (Subprogram) {
+    llvm::DIScope *Scope = Subprogram->getScope();
+    if (Scope) {
+      StringRef Dir = Scope->getDirectory();
+      if (Dir.empty() || Dir == ".")
+        ModuleName = Scope->getFilename().str();
+      else
+        ModuleName = Dir.str() + "/" + Scope->getFilename().str();
+    }
+  }
   std::pair<bool, SmallVector<BBClusterInfo, 4>> P =
-      BBSectionsProfileReader->getBBClusterInfoForFunction(MF.getName());
+      BBSectionsProfileReader->getBBClusterInfoForFunction(ModuleName, MF.getName());
   if (!P.first)
     return false;
 
   if (P.second.empty()) {
     // This indicates that sections are desired for all basic blocks of this
     // function. We clear the BBClusterInfo vector to denote this.
     V.clear();
     return true;

llvm/lib/CodeGen/BasicBlockSectionsProfileReader.cpp

 
 using namespace llvm;
 
 char BasicBlockSectionsProfileReader::ID = 0;
 INITIALIZE_PASS(BasicBlockSectionsProfileReader, "bbsections-profile-reader",
                 "Reads and parses a basic block sections profile.", false,
                 false)
 
-bool BasicBlockSectionsProfileReader::isFunctionHot(StringRef FuncName) const {
-  return getBBClusterInfoForFunction(FuncName).first;
+bool BasicBlockSectionsProfileReader::isFunctionHot(StringRef ModuleName,
+                                                    StringRef FuncName) const {
+  return getBBClusterInfoForFunction(ModuleName, FuncName).first;
 }
 
 std::pair<bool, SmallVector<BBClusterInfo>>
 BasicBlockSectionsProfileReader::getBBClusterInfoForFunction(
-    StringRef FuncName) const {
+    StringRef ModuleName, StringRef FuncName) const {
   std::pair<bool, SmallVector<BBClusterInfo>> cluster_info(false, {});
-  auto R = ProgramBBClusterInfo.find(getAliasName(FuncName));
-  if (R != ProgramBBClusterInfo.end()) {
-    cluster_info.second = R->second;
+  for (auto UsedModuleName : {ModuleName, StringRef("")}) {
+    auto R = ProgramBBClusterInfo.find(UsedModuleName);
+    if (R == ProgramBBClusterInfo.end())
+      continue;
+    auto S = R->second.find(getAliasName(UsedModuleName, FuncName));
+    if (S != R->second.end()) {
+      cluster_info.second = S->second;
       cluster_info.first = true;
     }
+  }
   return cluster_info;
 }
 
 // Basic Block Sections can be enabled for a subset of machine basic blocks.
 // This is done by passing a file containing names of functions for which basic
 // block sections are desired.  Additionally, machine basic block ids of the
 // functions can also be specified for a finer granularity. Moreover, a cluster
 // of basic blocks could be assigned to the same section.
 // A file with basic block sections for all of function main and three blocks
 // for function foo (of which 1 and 2 are placed in a cluster) looks like this:
 // ----------------------------
 // list.txt:
 // !main
 // !foo
 // !!1 2
 // !!4
-static Error getBBClusterInfo(const MemoryBuffer *MBuf,
-                              ProgramBBClusterInfoMapTy &ProgramBBClusterInfo,
-                              StringMap<StringRef> &FuncAliasMap) {
+static Error
+getBBClusterInfo(const MemoryBuffer *MBuf,
+                 std::vector<FunctionClusterInfo> &ProgramBBClusterInfo,
+                 StringMap<StringMap<int>> &ProfileIndexMap) {
   assert(MBuf);
   line_iterator LineIt(*MBuf, /*SkipBlanks=*/true, /*CommentMarker=*/'#');
 
   auto invalidProfileError = [&](auto Message) {
     return make_error<StringError>(
         Twine("Invalid profile " + MBuf->getBufferIdentifier() + " at line " +
               Twine(LineIt.line_number()) + ": " + Message),
         inconvertibleErrorCode());
   };
 
-  auto FI = ProgramBBClusterInfo.end();
+  FunctionClusterInfo CurrentClusterInfo;
 
   // Current cluster ID corresponding to this function.
   unsigned CurrentCluster = 0;
   // Current position in the current cluster.
   unsigned CurrentPosition = 0;
 
   // Temporary set to ensure every basic block ID appears once in the clusters
   // of a function.
   SmallSet<unsigned, 4> FuncBBIDs;
 
   for (; !LineIt.is_at_eof(); ++LineIt) {
     StringRef S(*LineIt);
     if (S[0] == '@')
       continue;
     // Check for the leading "!"
     if (!S.consume_front("!") || S.empty())
       break;
     // Check for second "!" which indicates a cluster of basic blocks.
     if (S.consume_front("!")) {
-      if (FI == ProgramBBClusterInfo.end())
+      if (CurrentClusterInfo.FunctionName.empty())
         return invalidProfileError(
             "Cluster list does not follow a function name specifier.");
       SmallVector<StringRef, 4> BBIDs;
       S.split(BBIDs, ' ');
       // Reset current cluster position.
       CurrentPosition = 0;
       for (auto BBIDStr : BBIDs) {
         unsigned long long BBID;
         if (getAsUnsignedInteger(BBIDStr, 10, BBID))
           return invalidProfileError(Twine("Unsigned integer expected: '") +
                                      BBIDStr + "'.");
         if (!FuncBBIDs.insert(BBID).second)
           return invalidProfileError(Twine("Duplicate basic block id found '") +
                                      BBIDStr + "'.");
         if (BBID == 0 && CurrentPosition)
           return invalidProfileError("Entry BB (0) does not begin a cluster.");
 
-        FI->second.emplace_back(
+        CurrentClusterInfo.Clusters.emplace_back(
             BBClusterInfo{((unsigned)BBID), CurrentCluster, CurrentPosition++});
       }
       CurrentCluster++;
     } else { // This is a function name specifier.
       // Function aliases are separated using '/'. We use the first function
       // name for the cluster info mapping and delegate all other aliases to
       // this one.
+      SmallVector<StringRef, 2> FunctionsAndFilename;
+      S.split(FunctionsAndFilename, ' ');
+      std::optional<StringRef> Filename = FunctionsAndFilename.size() == 1
+                                              ? std::nullopt
+                                              : FunctionAndModule[1];
       SmallVector<StringRef, 4> Aliases;
-      S.split(Aliases, '/');
-      for (size_t i = 1; i < Aliases.size(); ++i)
-        FuncAliasMap.try_emplace(Aliases[i], Aliases.front());
+      FunctionsAndFilename[0].split(Aliases, '/');
+      for (StringRef Alias : Aliases) {
+        ProfileIndexMap[DIFilename][Alias] = ProgramClusterInfo.size();
+      }
+      ProgramBBClusterInfo.push_back(CurrentClusterInfo);
 
       // Prepare for parsing clusters of this function name.
       // Start a new cluster map for this function name.
-      FI = ProgramBBClusterInfo.try_emplace(Aliases.front()).first;
+      FI = ProgramBBClusterInfo[ModuleName].try_emplace(Aliases.front()).first;
       CurrentCluster = 0;
       FuncBBIDs.clear();
     }
   }
   return Error::success();
 }
 
 void BasicBlockSectionsProfileReader::initializePass() {
   if (!MBuf)
     return;
   if (auto Err = getBBClusterInfo(MBuf, ProgramBBClusterInfo, FuncAliasMap))
     report_fatal_error(std::move(Err));
 }
 
 ImmutablePass *
 llvm::createBasicBlockSectionsProfileReaderPass(const MemoryBuffer *Buf) {
   return new BasicBlockSectionsProfileReader(Buf);
 }

llvm/lib/CodeGen/CodeGenPrepare.cpp

bool CodeGenPrepare::runOnFunction(Function &F) {
   BFI.reset(new BlockFrequencyInfo(F, *BPI, *LI));
   PSI = &getAnalysis<ProfileSummaryInfoWrapperPass>().getPSI();
   BBSectionsProfileReader =
       getAnalysisIfAvailable<BasicBlockSectionsProfileReader>();
   OptSize = F.hasOptSize();
   // Use the basic-block-sections profile to promote hot functions to .text.hot
   // if requested.
   if (BBSectionsGuidedSectionPrefix && BBSectionsProfileReader &&
-      BBSectionsProfileReader->isFunctionHot(F.getName())) {
+      BBSectionsProfileReader->isFunctionHot(F.getParent()->getSourceFileName(), F.getName())) {
     F.setSectionPrefix("hot");
   } else if (ProfileGuidedSectionPrefix) {
     // The hot attribute overwrites profile count based hotness while profile
     // counts based hotness overwrite the cold attribute.
     // This is a conservative behabvior.
     if (F.hasFnAttribute(Attribute::Hot) ||
         PSI->isFunctionHotInCallGraph(&F, *BFI))
       F.setSectionPrefix("hot");

llvm/test/CodeGen/X86/basic-block-sections-module1.ll

+;; Specify the correct module name.
+; RUN: echo '!test /path/to/dir/test_filename' > %t1
+; RUN: echo '!!0' >> %t1
+; RUN: llc < %s -mtriple=x86_64 -function-sections -basic-block-sections=%t1 | FileCheck %s -check-prefix=RIGHT-MODULE
+;; Specify no module name.
+; RUN: echo '!test' > %t2
+; RUN: echo '!!0' >> %t2
+; RUN: llc < %s -mtriple=x86_64 -function-sections -basic-block-sections=%t2  | FileCheck %s -check-prefix=NO-MODULE
+;; Specify incorrect module names.
+; RUN: echo '!test test_filename' > %t3
+; RUN: echo '!!0' >> %t3
+; RUN: llc < %s -mtriple=x86_64 -function-sections -basic-block-sections=%t3  | FileCheck %s -check-prefix=WRONG-MODULE
+; RUN: echo '!test ./path/to/dir/test_filename' > %t4
+; RUN: echo '!!0' >> %t4
+; RUN: llc < %s -mtriple=x86_64 -function-sections -basic-block-sections=%t4  | FileCheck %s -check-prefix=WRONG-MODULE
+
+define dso_local i32 @test(i32 noundef %0) #0 !dbg !10 {
+  %2 = alloca i32, align 4
+  %3 = alloca i32, align 4
+  store i32 %0, i32* %3, align 4
+  %4 = load i32, i32* %3, align 4
+  %5 = icmp slt i32 %4, 0
+  br i1 %5, label %6, label %7
+6:                                                ; preds = %1
+  store i32 -1, i32* %2, align 4
+  ret i32 0
+7:
+  ret i32 1
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1)
+!1 = !DIFile(filename: "test_filename", directory: "/path/to/dir")
+!2 = !{i32 7, !"Dwarf Version", i32 5}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!10 = distinct !DISubprogram(name: "test", scope: !1, unit: !0)
+
+;; Check that the split section is created when using the correct module name, or no module name.
+;
+; RIGHT-MODULE: .section        .text.split.test,"ax",@progbits
+; NO-MODULE: .section        .text.split.test,"ax",@progbits
+; WRONG-MODULE-NOT: .section    .text.split.test,"ax",@progbits

llvm/test/CodeGen/X86/basic-block-sections-module2.ll

+;; Specify the correct module name.
+; RUN: echo '!dummy' > %t1
+; RUN: echo '!!0 1' >> %1
+; RUN: echo '!test test_filename' >> %t1
+; RUN: echo '!!0' >> %t1
+; RUN: llc < %s -mtriple=x86_64 -function-sections -basic-block-sections=%t1  | FileCheck %s -check-prefix=RIGHT-MODULE
+; RUN: echo '!test ./test_filename' > %t2
+; RUN: echo '!!0' >> %t2
+; RUN: llc < %s -mtriple=x86_64 -function-sections -basic-block-sections=%t2  | FileCheck %s -check-prefix=WRONG-MODULE
+
+define dso_local i32 @test(i32 noundef %0) #0 !dbg !10 {
+  %2 = alloca i32, align 4
+  %3 = alloca i32, align 4
+  store i32 %0, i32* %3, align 4
+  %4 = load i32, i32* %3, align 4
+  %5 = icmp slt i32 %4, 0
+  br i1 %5, label %6, label %7
+6:                                                ; preds = %1
+  store i32 -1, i32* %2, align 4
+  ret i32 0
+7:
+  ret i32 1
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1)
+!1 = !DIFile(filename: "test_filename", directory: ".")
+!2 = !{i32 7, !"Dwarf Version", i32 5}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!10 = distinct !DISubprogram(name: "test", scope: !1, unit: !0)
+
+;; Check that the split section is created when using the correct module name, or no module name.
+;
+; RIGHT-MODULE: .section        .text.split.test,"ax",@progbits
+; WRONG-MODULE-NOT: .section        .text.split.test,"ax",@progbits