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

[Snehasish Kumar via Phabricator via llvm-commits]

snehasish added inline comments.


================
Comment at: llvm/lib/CodeGen/BasicBlockSections.cpp:92
+cl::opt<bool> DetectSourceDrift(
+    "detect-source-drift",
+    cl::desc("This checks if there is a fdo instr. profile hash "
----------------
Prefer naming the flag with a known prefix for easier identification. Eg. `bbsections-detect-source-drift`?


================
Comment at: llvm/lib/CodeGen/BasicBlockSections.cpp:327
+// on top of PGO optimized binaries and hence this check works well in practice.
+static bool checkSourceDrift(MachineFunction &MF) {
+  if (!DetectSourceDrift)
----------------
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.


================
Comment at: llvm/lib/CodeGen/BasicBlockSections.cpp:364
+  // regards to performance.
+  if (BBSectionsType == BasicBlockSection::List &&
+      checkSourceDrift(MF))
----------------
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.


================
Comment at: llvm/test/CodeGen/X86/bb_sections_hash_mismatch.ll:1
+; 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
----------------
Can you rename this file to be consistent with the other basic-block-sections tests? Eg. `basic-block-sections-source-drift.ll`


================
Comment at: llvm/test/CodeGen/X86/bb_sections_hash_mismatch.ll:2
+; 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
----------------
nit: Prefer hyphens to underscores to be consistent with the other basic block sections tests.


================
Comment at: llvm/test/CodeGen/X86/bb_sections_hash_mismatch.ll:18-19
+
+; SOURCE_DRIFT-NOT: .section .text
+; NO_SOURCE_DRIFT: .section .text
----------------
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?



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

https://reviews.llvm.org/D95593