Detect Source Drift with Propeller's basic-block-sections=list=

[tmsriram]

Summary

Detect Source Drift with Propeller.

Source Drift happens when the sources are updated after profiling the binary but before building the final optimized binary. If the source has changed since the profiles were obtained, optimizing basic blocks might be sub-optimal. This only applies to BasicBlockSection::List as it creates clusters of basic blocks using basic block ids. Source drift can invalidate these groupings leading to sub-optimal code generation with regards to performance.

PGO source drift for a particular function can be detected using function metadata added in D95495.

[snehasish]

llvm/lib/CodeGen/BasicBlockSections.cpp

92

Prefer naming the flag with a known prefix for easier identification. Eg. bbsections-detect-source-drift?

327

Perhaps hasInstrProfHashMismatch is a better name? I imagine in the future we will have more heuristics about detecting source drift where this is just one signal amongst many.

375

Should this be moved up before renumbering the blocks? I don't see a reason why we should renumber if we are going to bail out anyway.

llvm/test/CodeGen/X86/bb_sections_hash_mismatch.ll

1 ↗

(On Diff #319769)

Can you rename this file to be consistent with the other basic-block-sections tests? Eg. basic-block-sections-source-drift.ll

2 ↗

(On Diff #319769)

nit: Prefer hyphens to underscores to be consistent with the other basic block sections tests.

18–19 ↗

(On Diff #319769)

This was quite confusing at first glance.
SOURCE-DRIFT-NOT is checking for the absence of .section .text (when the check is enabled)
NO_SOURCE_DRIFT is checking for the absence option being enabled.

Perhaps the second one is clearer if named HASH-CHECK-DISABLED?
Also is it worthwhile to add a test case for the no annotation case (common case) to catch future regression?

[tmsriram]

Address all reviewer comments.

llvm/test/CodeGen/X86/bb_sections_hash_mismatch.ll

18–19 ↗

(On Diff #319769)

There are already test cases with .ll files and without the metadata that check for bb sections. Is that alright?

[snehasish]

lgtm

llvm/test/CodeGen/X86/bb_sections_hash_mismatch.ll

18–19 ↗

(On Diff #319769)

Yes, sgtm.

llvm/lib/CodeGen/BasicBlockSections.cpp

 // profiles and allows optimizations such as hugepage mapping to be applied at a
 // section granularity. Defaults to ".text.split." which is recognized by lld
 // via the `-z keep-text-section-prefix` flag.
 cl::opt<std::string> llvm::BBSectionsColdTextPrefix(
     "bbsections-cold-text-prefix",
     cl::desc("The text prefix to use for cold basic block clusters"),
     cl::init(".text.split."), cl::Hidden);
 
+cl::opt<bool> BBSectionsDetectSourceDrift(
+    "bbsections-detect-source-drift",
+    cl::desc("This checks if there is a fdo instr. profile hash "
+             "mismatch for this function"),
+    cl::init(true), cl::Hidden);
+
 namespace {
 
 // This struct represents the cluster information for a machine basic block.
 struct BBClusterInfo {
   // MachineBasicBlock ID.
   unsigned MBBNumber;
   // Cluster ID this basic block belongs to.
   unsigned ClusterID;
if (MBB.isBeginSection() && MBB.isEHPad()) {
       BuildMI(MBB, MI, DebugLoc(),
               MF.getSubtarget().getInstrInfo()->get(Noop.getOpcode()));
       return false;
     }
   }
   return true;
 }
 
+// This checks if the source of this function has drifted since this binary was
+// profiled previously.  For now, we are piggy backing on what PGO does to
+// detect this with instrumented profiles.  PGO emits an hash of the IR and
+// checks if the hash has changed.  Advanced basic block layout is usually done
+// on top of PGO optimized binaries and hence this check works well in practice.
+static bool hasInstrProfHashMismatch(MachineFunction &MF) {
+  if (!BBSectionsDetectSourceDrift)
+    return false;
+
+  const char MetadataName[] = "instr_prof_hash_mismatch";
+  auto *Existing = MF.getFunction().getMetadata(LLVMContext::MD_annotation);
+  if (Existing) {
+    MDTuple *Tuple = cast<MDTuple>(Existing);
+    for (auto &N : Tuple->operands())
+      if (cast<MDString>(N.get())->getString() == MetadataName)
+        return true;
+  }
+
+  return false;
+}
+
 bool BasicBlockSections::runOnMachineFunction(MachineFunction &MF) {
   auto BBSectionsType = MF.getTarget().getBBSectionsType();
   assert(BBSectionsType != BasicBlockSection::None &&
          "BB Sections not enabled!");
+
+  // Check for source drift.  If the source has changed since the profiles
+  // were obtained, optimizing basic blocks might be sub-optimal.
+  // This only applies to BasicBlockSection::List as it creates
+  // clusters of basic blocks using basic block ids. Source drift can
+  // invalidate these groupings leading to sub-optimal code generation with
+  // regards to performance.
+  if (BBSectionsType == BasicBlockSection::List &&
+      hasInstrProfHashMismatch(MF))
+    return true;
+
   // Renumber blocks before sorting them for basic block sections.  This is
   // useful during sorting, basic blocks in the same section will retain the
   // default order.  This renumbering should also be done for basic block
   // labels to match the profiles with the correct blocks.
   MF.RenumberBlocks();
 
   if (BBSectionsType == BasicBlockSection::Labels) {
     MF.setBBSectionsType(BBSectionsType);
     return true;
   }
 
   std::vector<Optional<BBClusterInfo>> FuncBBClusterInfo;
   if (BBSectionsType == BasicBlockSection::List &&
       !getBBClusterInfoForFunction(MF, FuncAliasMap, ProgramBBClusterInfo,
                                    FuncBBClusterInfo))
     return true;
   MF.setBBSectionsType(BBSectionsType);
   assignSections(MF, FuncBBClusterInfo);
 
   // We make sure that the cluster including the entry basic block precedes all
   // other clusters.
   auto EntryBBSectionID = MF.front().getSectionID();
 
   // Helper function for ordering BB sections as follows:
   //   * Entry section (section including the entry block).
   //   * Regular sections (in increasing order of their Number).

llvm/test/CodeGen/X86/basic-block-sections-source-drift.ll

+; RUN: echo "!foo" > %t.order.txt
+; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t.order.txt  | FileCheck --check-prefix=SOURCE-DRIFT %s
+; RUN: llc < %s -mtriple=x86_64-pc-linux -basic-block-sections=%t.order.txt -bbsections-detect-source-drift=false | FileCheck --check-prefix=HASH-CHECK-DISABLED %s
+
+define dso_local i32 @foo(i1 zeroext %0, i1 zeroext %1)  !annotation !1 {
+  br i1 %0, label %5, label %3
+
+3:                                                ; preds = %2
+  %4 = select i1 %1, i32 2, i32 0
+  ret i32 %4
+
+5:                                                ; preds = %2
+  ret i32 1
+}
+
+!1 = !{!"instr_prof_hash_mismatch"}
+
+; SOURCE-DRIFT-NOT: .section .text
+; HASH-CHECK-DISABLED: .section .text