[PATCH] D116180: [InstrProf] Add single byte coverage mode

[David Li via Phabricator via llvm-commits]

davidxl added inline comments.


================
Comment at: llvm/docs/LangRef.rst:13140
+
+      declare void @llvm.instrprof.cover(i8* <name>, i64 <hash>)
+
----------------
probably name it llvm.instrprof.entry_cover to reflect the semantics. In theory you can have block level coverage mode too.


================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:1179
 
-  GlobalVariable *getName() const {
-    return cast<GlobalVariable>(
----------------
Split out this in different patch?


================
Comment at: llvm/include/llvm/ProfileData/InstrProfReader.h:484
 
+  bool useSingleByteEntryCoverage() const override {
+    return (FormatVersion & VARIANT_MASK_BYTE_ENTRY_COVERAGE) != 0;
----------------
entry coverage is compatiable with IR pgo -- so perhaps add an assertion somewhere.


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:942
+  GlobalVariable *CounterPtr;
+  if (isa<InstrProfCoverInst>(Inc)) {
+    auto *CounterTy = Type::getInt8Ty(Ctx);
----------------
Split out the change into a helper function to make it cleaner.


================
Comment at: llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp:258
 
+static cl::opt<bool> CoverageInstrumentation(
+    "pgo-coverage-instrumentation", cl::init(false), cl::Hidden, cl::ZeroOrMore,
----------------
make the variable name matching the option name


================
Comment at: llvm/test/Transforms/PGOProfile/coverage.ll:12
+if.then:
+  ; CHECK-NOT: llvm.instrprof.cover(
+  %add = add nsw i32 %i, 2
----------------
Just check there is one instance of instrinsic call per function?


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:2093
 
 static int showInstrProfile(const std::string &Filename, bool ShowCounts,
                             uint32_t TopN, bool ShowIndirectCallTargets,
----------------
Add a test case for the coverage mode.


================
Comment at: llvm/tools/llvm-profdata/llvm-profdata.cpp:2159
+    if (ShowCovered)
+      if (std::any_of(Func.Counts.begin(), Func.Counts.end(),
+                      [](uint64_t C) { return C; }))
----------------
should probably by pass the rest of dump in coverage mode.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116180