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

[Kyungwoo Lee via Phabricator via llvm-commits]

kyulee added a comment.

LGTM. I'm approving this but suggest for other reviewers'. Also I left a few comments about naming counters that may follow up.



================
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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116180