[PATCH] D95593: Detect Source Drift with Propeller's basic-block-sections=list=

[Sriraman Tallam via Phabricator via llvm-commits]

tmsriram created this revision.
tmsriram added reviewers: rahmanl, snehasish, efriedma, davidxl, shenhan.
Herald added subscribers: wenlei, pengfei, hiraditya.
tmsriram requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

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 <https://reviews.llvm.org/D95495>.


https://reviews.llvm.org/D95593

Files:
  llvm/lib/CodeGen/BasicBlockSections.cpp
  llvm/test/CodeGen/X86/bb_sections_hash_mismatch.ll


Index: llvm/test/CodeGen/X86/bb_sections_hash_mismatch.ll
===================================================================
--- /dev/null
+++ llvm/test/CodeGen/X86/bb_sections_hash_mismatch.ll
@@ -0,0 +1,19 @@
+; 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 -detect-source-drift=false | FileCheck --check-prefix=NO_SOURCE_DRIFT %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
+; NO_SOURCE_DRIFT: .section .text
Index: llvm/lib/CodeGen/BasicBlockSections.cpp
===================================================================
--- llvm/lib/CodeGen/BasicBlockSections.cpp
+++ llvm/lib/CodeGen/BasicBlockSections.cpp
@@ -88,6 +88,12 @@
     cl::desc("The text prefix to use for cold basic block clusters"),
     cl::init(".text.split."), cl::Hidden);
 
+cl::opt<bool> DetectSourceDrift(
+    "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.
@@ -313,10 +319,31 @@
   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 checkSourceDrift(MachineFunction &MF) {
+  if (!DetectSourceDrift)
+    return false;
+
+  const char MetadataName[] = "instr_prof_hash_mismatch";
+  if (MDTuple *Tuple =
+      cast<MDTuple>(MF.getFunction().getMetadata(LLVMContext::MD_annotation))) {
+    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!");
+
   // 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
@@ -328,6 +355,16 @@
     return true;
   }
 
+  // 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 &&
+      checkSourceDrift(MF))
+    return true;
+
   std::vector<Optional<BBClusterInfo>> FuncBBClusterInfo;
   if (BBSectionsType == BasicBlockSection::List &&
       !getBBClusterInfoForFunction(MF, FuncAliasMap, ProgramBBClusterInfo,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D95593.319769.patch
Type: text/x-patch
Size: 3616 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210128/63cb88f0/attachment.bin>