[PATCH] D116180: [InstrProf] Add single byte coverage mode
Ellis Hoag via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jan 27 14:14:49 PST 2022
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
More information about the llvm-commits
mailing list