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

[Ellis Hoag via Phabricator via llvm-commits]

ellis added inline comments.


================
Comment at: compiler-rt/lib/profile/InstrProfilingMerge.c:161
+        // A value of zero signifies the function is covered.
+        DstCounters[I] &= SrcCounters[I];
+      } else {
----------------
davidxl wrote:
> is there a test case for this?
Added the test `instrprof-merge-entry-cover.c` and found a bug in `__llvm_profile_reset_counters()`. Thanks for the reminder!


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:696
 
-void InstrProfiling::lowerIncrement(InstrProfIncrementInst *Inc) {
-  GlobalVariable *Counters = getOrCreateRegionCounters(Inc);
-
-  IRBuilder<> Builder(Inc);
-  uint64_t Index = Inc->getIndex()->getZExtValue();
-  Value *Addr = Builder.CreateConstInBoundsGEP2_32(Counters->getValueType(),
-                                                   Counters, 0, Index);
-
-  if (isRuntimeCounterRelocationEnabled()) {
-    Type *Int64Ty = Type::getInt64Ty(M->getContext());
-    Type *Int64PtrTy = Type::getInt64PtrTy(M->getContext());
-    Function *Fn = Inc->getParent()->getParent();
-    Instruction &I = Fn->getEntryBlock().front();
-    LoadInst *LI = dyn_cast<LoadInst>(&I);
-    if (!LI) {
-      IRBuilder<> Builder(&I);
-      GlobalVariable *Bias =
-          M->getGlobalVariable(getInstrProfCounterBiasVarName());
-      if (!Bias) {
-        // Compiler must define this variable when runtime counter relocation
-        // is being used. Runtime has a weak external reference that is used
-        // to check whether that's the case or not.
-        Bias = new GlobalVariable(
-            *M, Int64Ty, false, GlobalValue::LinkOnceODRLinkage,
-            Constant::getNullValue(Int64Ty), getInstrProfCounterBiasVarName());
-        Bias->setVisibility(GlobalVariable::HiddenVisibility);
-        // A definition that's weak (linkonce_odr) without being in a COMDAT
-        // section wouldn't lead to link errors, but it would lead to a dead
-        // data word from every TU but one. Putting it in COMDAT ensures there
-        // will be exactly one data slot in the link.
-        if (TT.supportsCOMDAT())
-          Bias->setComdat(M->getOrInsertComdat(Bias->getName()));
-      }
-      LI = Builder.CreateLoad(Int64Ty, Bias);
+Value *InstrProfiling::getCounterAddress(InstrProfInstBase *I) {
+  auto *Counters = getOrCreateRegionCounters(I);
----------------
kyulee wrote:
> Strictly speaking, it's not a counter for coverage because a store instead of an adder will be used. Same as in the description of intrinsic, and all other api names using counters.
> A probe or something might be a better name. However, it seems very invasive to change all the counter name, which may confuse the current (main) semantic for edge counter.
> I suggest to just keep as it but I'm also open to other reviewer's opinions.
Yeah, I would like to use "probe", but "counters" is used all of the place in the source. I can bring this up in https://llvm.discourse.group/ to see if we are willing to make this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116180