Skip to content

[Coverage] Make additional counters available for BranchRegion. NFC. #112730

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8 changes: 7 additions & 1 deletion clang/lib/CodeGen/CodeGenFunction.h
Original file line number Diff line number Diff line change
Expand Up @@ -1632,11 +1632,17 @@ class CodeGenFunction : public CodeGenTypeCache {
/// Increment the profiler's counter for the given statement by \p StepV.
/// If \p StepV is null, the default increment is 1.
void incrementProfileCounter(const Stmt *S, llvm::Value *StepV = nullptr) {
incrementProfileCounter(false, S, false, StepV);
}

void incrementProfileCounter(bool UseSkipPath, const Stmt *S,
bool UseBoth = false,
llvm::Value *StepV = nullptr) {
if (CGM.getCodeGenOpts().hasProfileClangInstr() &&
!CurFn->hasFnAttribute(llvm::Attribute::NoProfile) &&
!CurFn->hasFnAttribute(llvm::Attribute::SkipProfile)) {
auto AL = ApplyDebugLocation::CreateArtificial(*this);
PGO.emitCounterSetOrIncrement(Builder, S, StepV);
PGO.emitCounterSetOrIncrement(Builder, S, UseSkipPath, UseBoth, StepV);
}
PGO.setCurrentStmt(S);
}
Expand Down
32 changes: 30 additions & 2 deletions clang/lib/CodeGen/CodeGenPGO.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1137,6 +1137,19 @@ void CodeGenPGO::emitCounterRegionMapping(const Decl *D) {
if (CoverageMapping.empty())
return;

// Scan max(FalseCnt) and update NumRegionCounters.
unsigned MaxNumCounters = NumRegionCounters;
for (const auto [_, V] : *RegionCounterMap) {
auto HasCounters = V.getIsCounterPair();
assert((!HasCounters.first ||
MaxNumCounters > (V.first & CounterPair::Mask)) &&
"TrueCnt should not be reassigned");
if (HasCounters.second)
MaxNumCounters =
std::max(MaxNumCounters, (V.second & CounterPair::Mask) + 1);
}
NumRegionCounters = MaxNumCounters;

CGM.getCoverageMapping()->addFunctionMappingRecord(
FuncNameVar, FuncName, FunctionHash, CoverageMapping);
}
Expand Down Expand Up @@ -1192,11 +1205,26 @@ std::pair<bool, bool> CodeGenPGO::getIsCounterPair(const Stmt *S) const {
}

void CodeGenPGO::emitCounterSetOrIncrement(CGBuilderTy &Builder, const Stmt *S,
bool UseSkipPath, bool UseBoth,
llvm::Value *StepV) {
if (!RegionCounterMap || !Builder.GetInsertBlock())
if (!RegionCounterMap)
return;

unsigned Counter = (*RegionCounterMap)[S].first;
unsigned Counter;
auto &TheMap = (*RegionCounterMap)[S];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static_cast?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mention the use of auto?

auto IsCounter = TheMap.getIsCounterPair();
if (!UseSkipPath) {
if (!IsCounter.first)
return;
Counter = (TheMap.first & CounterPair::Mask);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch introduces a lot of code like

(X & CounterPair::Mask)

Would it make sense to create a small helper function to do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I encapsulated it. See #112724.

} else {
if (!IsCounter.second)
return;
Counter = (TheMap.second & CounterPair::Mask);
}

if (!Builder.GetInsertBlock())
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When can this happen? Comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was the result of splitting the condition.

if (!RegionCounterMap || !Builder.GetInsertBlock())
    return;

to

if (!RegionCounterMap)
    return;

auto &TheMap = (*RegionCounterMap)[S]; // S should be allocated (as an initial value)
...

if (!Builder.GetInsertBlock())
    return;

return;

// Make sure that pointer to global is passed in with zero addrspace
// This is relevant during GPU profiling
Expand Down
1 change: 1 addition & 0 deletions clang/lib/CodeGen/CodeGenPGO.h
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ class CodeGenPGO {
public:
std::pair<bool, bool> getIsCounterPair(const Stmt *S) const;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function could use some documentation, or a better name.

This can happen in a later commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't name better.

return {I->second.Executed.hasValue(), I->second.Skipped.hasValue()};

It is defined in #112724.

void emitCounterSetOrIncrement(CGBuilderTy &Builder, const Stmt *S,
bool UseFalsePath, bool UseBoth,
llvm::Value *StepV);
void emitMCDCTestVectorBitmapUpdate(CGBuilderTy &Builder, const Expr *S,
Address MCDCCondBitmapAddr,
Expand Down
47 changes: 37 additions & 10 deletions clang/lib/CodeGen/CoverageMappingGen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -884,6 +884,9 @@ struct CounterCoverageMappingBuilder
/// The map of statements to count values.
llvm::DenseMap<const Stmt *, CounterPair> &CounterMap;

CounterExpressionBuilder::SubstMap MapToExpand;
unsigned NextCounterNum;

MCDC::State &MCDCState;

/// A stack of currently live regions.
Expand Down Expand Up @@ -919,15 +922,11 @@ struct CounterCoverageMappingBuilder

/// Return a counter for the sum of \c LHS and \c RHS.
Counter addCounters(Counter LHS, Counter RHS, bool Simplify = true) {
assert(!llvm::EnableSingleByteCoverage &&
"cannot add counters when single byte coverage mode is enabled");
return Builder.add(LHS, RHS, Simplify);
}

Counter addCounters(Counter C1, Counter C2, Counter C3,
bool Simplify = true) {
assert(!llvm::EnableSingleByteCoverage &&
"cannot add counters when single byte coverage mode is enabled");
return addCounters(addCounters(C1, C2, Simplify), C3, Simplify);
}

Expand All @@ -940,15 +939,35 @@ struct CounterCoverageMappingBuilder

std::pair<Counter, Counter> getBranchCounterPair(const Stmt *S,
Counter ParentCnt) {
Counter ExecCnt = getRegionCounter(S);
return {ExecCnt, Builder.subtract(ParentCnt, ExecCnt)};
auto &TheMap = CounterMap[S];
auto ExecCnt = Counter::getCounter(TheMap.first);
auto SkipExpr = Builder.subtract(ParentCnt, ExecCnt);

if (!llvm::EnableSingleByteCoverage || !SkipExpr.isExpression()) {
assert(
!TheMap.getIsCounterPair().second &&
"SkipCnt shouldn't be allocated but refer to an existing counter.");
return {ExecCnt, SkipExpr};
}

// Assign second if second is not assigned yet.
if (!TheMap.getIsCounterPair().second)
TheMap.second = NextCounterNum++;

Counter SkipCnt = Counter::getCounter(TheMap.second);
MapToExpand[SkipCnt] = SkipExpr;
return {ExecCnt, SkipCnt};
}

/// Returns {TrueCnt,FalseCnt} for "implicit default".
/// FalseCnt is considered as the False count on SwitchStmt.
std::pair<Counter, Counter>
getSwitchImplicitDefaultCounterPair(const Stmt *Cond, Counter ParentCount,
Counter CaseCountSum) {
if (llvm::EnableSingleByteCoverage)
return {Counter::getZero(), // Folded
Counter::getCounter(CounterMap[Cond].second = NextCounterNum++)};

// Simplify is skipped while building the counters above: it can get
// really slow on top of switches with thousands of cases. Instead,
// trigger simplification by adding zero to the last counter.
Expand All @@ -962,6 +981,11 @@ struct CounterCoverageMappingBuilder
if (OutCount == ParentCount)
return true;

// Try comaparison with pre-replaced expressions.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put a small example in the comment here?

if (Builder.subst(Builder.subtract(OutCount, ParentCount), MapToExpand)
.isZero())
return true;

return false;
}

Expand Down Expand Up @@ -1175,12 +1199,14 @@ struct CounterCoverageMappingBuilder
/// and add it to the function's SourceRegions.
/// Returns Counter that corresponds to SC.
Counter createSwitchCaseRegion(const SwitchCase *SC, Counter ParentCount) {
Counter TrueCnt = getRegionCounter(SC);
Counter FalseCnt = (llvm::EnableSingleByteCoverage
? Counter::getZero() // Folded
: subtractCounters(ParentCount, TrueCnt));
// Push region onto RegionStack but immediately pop it (which adds it to
// the function's SourceRegions) because it doesn't apply to any other
// source other than the SwitchCase.
Counter TrueCnt = getRegionCounter(SC);
popRegions(pushRegion(TrueCnt, getStart(SC), SC->getColonLoc(),
subtractCounters(ParentCount, TrueCnt)));
popRegions(pushRegion(TrueCnt, getStart(SC), SC->getColonLoc(), FalseCnt));
return TrueCnt;
}

Expand Down Expand Up @@ -1447,7 +1473,8 @@ struct CounterCoverageMappingBuilder
llvm::DenseMap<const Stmt *, CounterPair> &CounterMap,
MCDC::State &MCDCState, SourceManager &SM, const LangOptions &LangOpts)
: CoverageMappingBuilder(CVM, SM, LangOpts), CounterMap(CounterMap),
MCDCState(MCDCState), MCDCBuilder(CVM.getCodeGenModule(), MCDCState) {}
NextCounterNum(CounterMap.size()), MCDCState(MCDCState),
MCDCBuilder(CVM.getCodeGenModule(), MCDCState) {}

/// Write the mapping data to the output stream
void write(llvm::raw_ostream &OS) {
Expand Down
3 changes: 3 additions & 0 deletions llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -637,6 +637,9 @@ static unsigned getMaxCounterID(const CounterMappingContext &Ctx,
unsigned MaxCounterID = 0;
for (const auto &Region : Record.MappingRegions) {
MaxCounterID = std::max(MaxCounterID, Ctx.getMaxCounterID(Region.Count));
if (Region.isBranch())
MaxCounterID =
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we have a updateMaxCounterID helper? I think this happens in a couple places in the code now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there was no idea in initial designs to increase Counters dynamically. It shall be improved in the future.

std::max(MaxCounterID, Ctx.getMaxCounterID(Region.FalseCount));
}
return MaxCounterID;
}
Expand Down
Loading