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

[Ellis Hoag via Phabricator via llvm-commits]

ellis added inline comments.


================
Comment at: compiler-rt/test/profile/instrprof-coverage.c:1
+// RUN: %clang_pgogen -mllvm -pgo-coverage-instrumentation %s -o %t.out
+// RUN: env LLVM_PROFILE_FILE=%t.profraw %run %t.out
----------------
kyulee wrote:
> Can you add a test case using the debug data `-debug-info`?
I've added this to the above test `instrprof-debug-info-correlate.c`.


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


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


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:947
+    std::vector<Constant *> InitialValues(NumCounters,
+                                          Constant::getAllOnesValue(CounterTy));
+    CounterPtr = new GlobalVariable(
----------------
kyulee wrote:
> Can we just use `Constant::getAllOnesValue()` like `Constant::getNullValue()` with the array type?
Unfortunately, `ConstantArray::get()` takes an array so we cannot give it just a `Constant`.

https://llvm.org/doxygen/classllvm_1_1ConstantArray.html#a0900dacdc7ad8e3ea0cc92993c7fd422

Also, `getAllOnesValue()` does not appear to accept an array type yet.


================
Comment at: llvm/test/Transforms/PGOProfile/coverage.ll:12
+if.then:
+  ; CHECK-NOT: llvm.instrprof.cover(
+  %add = add nsw i32 %i, 2
----------------
davidxl wrote:
> Just check there is one instance of instrinsic call per function?
Yeah, this is just a sanity check that it isn't called in other blocks.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116180