[BOLT] Set valid index for functions with profiles

[yota9]

Summary

Some of the passes that calculates tentative layout like LongJmp and
Golang are expecting that only functions with valid index will be
located in hot text section. But currently functions with valid profiles
and not set index are breaking this logic, to fix this we can move the
hasValidProfile() condition from AssignSections pass to ReorderFunctions.

Vladislav Khmelevsky,
Advanced Software Technology Lab, Huawei

[yota9]

Fix comment

[maksfb]

Some of the passes that calculates tentative layout like LongJmp and Golang are expecting that only functions with valid index will be located in hot text section.

Do these passes care about functions written to cold text at all? I wonder if we need to assign indices to all functions regardless of the profile.

bolt/lib/Passes/ReorderFunctions.cpp

132

Or KV.

[yota9]

In D127223#3564746, @maksfb wrote:

Some of the passes that calculates tentative layout like LongJmp and Golang are expecting that only functions with valid index will be located in hot text section.

Do these passes care about functions written to cold text at all? I wonder if we need to assign indices to all functions regardless of the profile.

Sure, they are. E.g. we need to know exact layout to perform all the relaxations in jumps/calls in/to hot/cold sections.
We don't need to assign indices to cold functions since we get them from getSortedFunctions() vector one by one. But we need to know the boundaries between hot and cold parts and should know exactly in which section the function goest to or its part. Using the indices we easily see that the first part of the sorted functions vector contains hot/split functions and the second part only cold functions and easily can perform right layout calculations.

[yota9]

address comment

[maksfb]

We were considering adding another level of "hotness" for basic blocks and maybe for functions. I.e. potentially we might have hot/cold/frozen split if it proves to be beneficial. Then relying on implicit assumptions about indices will become too fragile. Something to keep in mind.

bolt/lib/Passes/BinaryPasses.cpp

void AssignSections::runOnFunctions(BinaryContext &BC) {
   for (auto &BFI : BC.getBinaryFunctions()) {
     BinaryFunction &Function = BFI.second;
     if (opts::isHotTextMover(Function)) {
       Function.setCodeSectionName(BC.getHotTextMoverSectionName());
       Function.setColdCodeSectionName(BC.getHotTextMoverSectionName());
       continue;
     }
 
-    if (!UseColdSection || Function.hasValidIndex() ||
-        Function.hasValidProfile())
+    if (!UseColdSection || Function.hasValidIndex())
       Function.setCodeSectionName(BC.getMainCodeSectionName());
     else
       Function.setCodeSectionName(BC.getColdCodeSectionName());
 
     if (Function.isSplit())
       Function.setColdCodeSectionName(BC.getColdCodeSectionName());
   }
 }

bolt/lib/Passes/ReorderFunctions.cpp

void ReorderFunctions::reorder(std::vector<Cluster> &&Clusters,
   for (const Cluster &Cluster : Clusters) {
     for (const NodeId FuncId : Cluster.targets()) {
       Cg.nodeIdToFunc(FuncId)->setIndex(Index++);
       FuncAddr[FuncId] = TotalSize;
       TotalSize += Cg.size(FuncId);
     }
   }
 
+  // Assign valid index for functions with valid profile.
+  for (auto &It : BFs) {
+    BinaryFunction &BF = It.second;
+    if (!BF.hasValidIndex() && BF.hasValidProfile())
+      BF.setIndex(Index++);
+  }
+
   if (opts::ReorderFunctions == RT_NONE)
     return;
 
   if (opts::Verbosity == 0) {
 #ifndef NDEBUG
     if (!DebugFlag || !isCurrentDebugType("hfsort"))
       return;
 #else