Skip to content

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

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

Open
wants to merge 62 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 51 commits
Commits
Show all changes
62 commits
Select commit Hold shift + click to select a range
618e639
Introduce CounterExpressionBuilder::replace(C, Map)
chapuni Oct 16, 2024
fc697f0
[Coverage] Introduce `getBranchCounterPair()`. NFC.
chapuni Oct 16, 2024
e4172ca
Introduce the type `CounterPair` for RegionCounterMap
chapuni Oct 16, 2024
12abd89
Merge branches 'users/chapuni/cov/single/getpair', 'users/chapuni/cov…
chapuni Oct 17, 2024
5e46059
[Coverage] Make additional counters available for BranchRegion. NFC.
chapuni Oct 17, 2024
ad13691
Rewind changes for folding
chapuni Oct 18, 2024
209ea4c
Update comments
chapuni Oct 20, 2024
f0afd04
Use initializer statements
chapuni Oct 20, 2024
d4518a4
Merge branches 'users/chapuni/cov/single/getpair' and 'users/chapuni/…
chapuni Oct 20, 2024
05df8df
Merge branch 'users/chapuni/cov/single/nextcount-base' into users/cha…
chapuni Oct 20, 2024
be516fa
`first` may be cancelled.
chapuni Oct 20, 2024
03cfce1
CGF::markStmtAsUsed
chapuni Oct 23, 2024
afc8481
CGF.markStmtMaybeUsed for binop
chapuni Oct 23, 2024
58feee3
Merge branch 'main' into users/chapuni/cov/single/getpair
chapuni Oct 27, 2024
ab84f17
Introduce skeleton getSwitchImplicitDefaultCounter()
chapuni Oct 27, 2024
804d330
Merge branches 'users/chapuni/cov/single/pair' and 'users/chapuni/cov…
chapuni Oct 27, 2024
2c29f5d
Merge branch 'users/chapuni/cov/single/nextcount-base' into HEAD
chapuni Oct 27, 2024
a460885
Update getSwitchImplicitDefaultCounter
chapuni Oct 27, 2024
0285394
Don't allocate second if SkipExpr isn't Expr.
chapuni Oct 27, 2024
ce7c17d
Merge branch 'main' into users/chapuni/cov/single/pair
chapuni Dec 21, 2024
631bc35
Merge branch 'main' into users/chapuni/cov/single/replace
chapuni Dec 21, 2024
ed700c2
s/replace/subst/
chapuni Dec 21, 2024
9d3c3b0
Merge branch 'main' into users/chapuni/cov/single/getpair
chapuni Dec 21, 2024
dbcf896
getSwitchImplicitDefaultCounterPair
chapuni Dec 21, 2024
2cb6395
Merge branch 'users/chapuni/cov/single/pair' into users/chapuni/cov/s…
chapuni Dec 21, 2024
702a72e
Merge branch 'users/chapuni/cov/single/getpair' into users/chapuni/co…
chapuni Dec 21, 2024
36465dc
Merge branch 'users/chapuni/cov/single/replace' into users/chapuni/co…
chapuni Dec 21, 2024
2413b83
Merge branch 'users/chapuni/cov/single/nextcount-base' into users/cha…
chapuni Dec 21, 2024
c0785e9
Fold either in switchcase
chapuni Dec 21, 2024
bd1d96b
Introduce CounterMappingRegion::isBranch(). NFC.
chapuni Dec 23, 2024
19edcd3
Merge branch 'main' into users/chapuni/cov/single/getpair
chapuni Dec 24, 2024
4e41b99
Introduce BranchCounterPair
chapuni Dec 24, 2024
63dbfb3
Merge branch 'main' into users/chapuni/cov/single/pair
chapuni Dec 24, 2024
306d77f
void verifyCounterMap() const
chapuni Dec 24, 2024
a4f05c7
Introduce the dedicated class CounterPair instead of std::pair
chapuni Dec 24, 2024
1f18ab9
Merge branches 'users/chapuni/cov/single/getpair' and 'users/chapuni/…
chapuni Dec 24, 2024
b7ae558
Merge branch 'users/chapuni/cov/single/nextcount-base' into users/cha…
chapuni Dec 24, 2024
f6c5f40
Catch up the merge
chapuni Dec 24, 2024
aca86d4
Introduce {UseExecPath, UseSkipPath} instead of {false, true}
chapuni Dec 24, 2024
28c568a
Add a test
chapuni Jan 6, 2025
d92a9d9
Prune redundant logic
chapuni Jan 6, 2025
93fb420
Merge branch 'users/chapuni/cov/single/replace' into users/chapuni/co…
chapuni Jan 6, 2025
6f8681c
Merge branch 'users/chapuni/cov/single/nextcount-base' into users/cha…
chapuni Jan 6, 2025
82f2e92
Expand RHS of MapToExpand. This will prevent recursion.
chapuni Jan 6, 2025
b90fdf6
Append an explanation in the comment
chapuni Jan 6, 2025
d854fb1
Rewind switch DefaultCase. (to #113112)
chapuni Jan 7, 2025
bac2967
Enable addCounters
chapuni Jan 7, 2025
6bae87d
Get rid of structual bindings
chapuni Jan 8, 2025
c8edf58
Flatten with getBranchCounterPair(SkipCntForOld)
chapuni Jan 8, 2025
ec6892d
Merge branch 'users/chapuni/cov/single/getpair' into users/chapuni/co…
chapuni Jan 8, 2025
9b99dde
Merge branch 'users/chapuni/cov/single/nextcount-base' into users/cha…
chapuni Jan 8, 2025
f2ba219
Update comments
chapuni Jan 8, 2025
97015cb
Decorate the mock
chapuni Jan 8, 2025
9a40d20
Will be pruned after the migration of SingleByte.
chapuni Jan 9, 2025
b548e71
Add comments
chapuni Jan 9, 2025
13fbcde
Merge branch 'main' into users/chapuni/cov/single/replace
chapuni Jan 9, 2025
789eeab
Merge branch 'main' into users/chapuni/cov/single/getpair
chapuni Jan 9, 2025
8683882
Merge branch 'users/chapuni/cov/single/replace' into users/chapuni/co…
chapuni Jan 9, 2025
de5756c
Merge branch 'main' into users/chapuni/cov/single/pair
chapuni Jan 9, 2025
6d16b1c
Merge branch 'users/chapuni/cov/single/getpair' into users/chapuni/co…
chapuni Jan 9, 2025
0aa930a
Merge branch 'users/chapuni/cov/single/pair' into users/chapuni/cov/s…
chapuni Jan 9, 2025
fea7da1
Merge branch 'users/chapuni/cov/single/nextcount-base' into users/cha…
chapuni Jan 9, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion clang/lib/CodeGen/CGDecl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -361,6 +361,8 @@ CodeGenFunction::AddInitializerToStaticVarDecl(const VarDecl &D,
return GV;
}

PGO.markStmtMaybeUsed(D.getInit()); // FIXME: Too lazy

#ifndef NDEBUG
CharUnits VarSize = CGM.getContext().getTypeSizeInChars(D.getType()) +
D.getFlexibleArrayInitChars(getContext());
Expand Down Expand Up @@ -1868,7 +1870,10 @@ void CodeGenFunction::EmitAutoVarInit(const AutoVarEmission &emission) {
// If we are at an unreachable point, we don't need to emit the initializer
// unless it contains a label.
if (!HaveInsertPoint()) {
if (!Init || !ContainsLabel(Init)) return;
if (!Init || !ContainsLabel(Init)) {
PGO.markStmtMaybeUsed(Init);
return;
}
EnsureInsertPoint();
}

Expand Down Expand Up @@ -1979,6 +1984,8 @@ void CodeGenFunction::EmitAutoVarInit(const AutoVarEmission &emission) {
return EmitExprAsInit(Init, &D, lv, capturedByInit);
}

PGO.markStmtMaybeUsed(Init);

if (!emission.IsConstantAggregate) {
// For simple scalar/complex initialization, store the value directly.
LValue lv = MakeAddrLValue(Loc, type);
Expand Down
1 change: 1 addition & 0 deletions clang/lib/CodeGen/CGExpr.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5148,6 +5148,7 @@ std::optional<LValue> HandleConditionalOperatorLValueSimpleCase(
// If the true case is live, we need to track its region.
if (CondExprBool)
CGF.incrementProfileCounter(E);
CGF.markStmtMaybeUsed(Dead);
// If a throw expression we emit it and return an undefined lvalue
// because it can't be used.
if (auto *ThrowExpr = dyn_cast<CXXThrowExpr>(Live->IgnoreParens())) {
Expand Down
15 changes: 11 additions & 4 deletions clang/lib/CodeGen/CGExprScalar.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5003,7 +5003,8 @@ Value *ScalarExprEmitter::VisitBinLAnd(const BinaryOperator *E) {
CGF.incrementProfileCounter(E->getRHS());
CGF.EmitBranch(FBlock);
CGF.EmitBlock(FBlock);
}
} else
CGF.markStmtMaybeUsed(E->getRHS());

CGF.MCDCLogOpStack.pop_back();
// If the top of the logical operator nest, update the MCDC bitmap.
Expand All @@ -5015,8 +5016,10 @@ Value *ScalarExprEmitter::VisitBinLAnd(const BinaryOperator *E) {
}

// 0 && RHS: If it is safe, just elide the RHS, and return 0/false.
if (!CGF.ContainsLabel(E->getRHS()))
if (!CGF.ContainsLabel(E->getRHS())) {
CGF.markStmtMaybeUsed(E->getRHS());
return llvm::Constant::getNullValue(ResTy);
}
}

// If the top of the logical operator nest, reset the MCDC temp to 0.
Expand Down Expand Up @@ -5143,7 +5146,8 @@ Value *ScalarExprEmitter::VisitBinLOr(const BinaryOperator *E) {
CGF.incrementProfileCounter(E->getRHS());
CGF.EmitBranch(FBlock);
CGF.EmitBlock(FBlock);
}
} else
CGF.markStmtMaybeUsed(E->getRHS());

CGF.MCDCLogOpStack.pop_back();
// If the top of the logical operator nest, update the MCDC bitmap.
Expand All @@ -5155,8 +5159,10 @@ Value *ScalarExprEmitter::VisitBinLOr(const BinaryOperator *E) {
}

// 1 || RHS: If it is safe, just elide the RHS, and return 1/true.
if (!CGF.ContainsLabel(E->getRHS()))
if (!CGF.ContainsLabel(E->getRHS())) {
CGF.markStmtMaybeUsed(E->getRHS());
return llvm::ConstantInt::get(ResTy, 1);
}
}

// If the top of the logical operator nest, reset the MCDC temp to 0.
Expand Down Expand Up @@ -5280,6 +5286,7 @@ VisitAbstractConditionalOperator(const AbstractConditionalOperator *E) {
CGF.incrementProfileCounter(E);
}
Value *Result = Visit(live);
CGF.markStmtMaybeUsed(dead);

// If the live part is a throw expression, it acts like it has a void
// type, so evaluating it returns a null Value*. However, a conditional
Expand Down
3 changes: 3 additions & 0 deletions clang/lib/CodeGen/CGStmt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ void CodeGenFunction::EmitStmt(const Stmt *S, ArrayRef<const Attr *> Attrs) {
// Verify that any decl statements were handled as simple, they may be in
// scope of subsequent reachable statements.
assert(!isa<DeclStmt>(*S) && "Unexpected DeclStmt!");
PGO.markStmtMaybeUsed(S);
return;
}

Expand Down Expand Up @@ -869,6 +870,7 @@ void CodeGenFunction::EmitIfStmt(const IfStmt &S) {
RunCleanupsScope ExecutedScope(*this);
EmitStmt(Executed);
}
PGO.markStmtMaybeUsed(Skipped);
return;
}
}
Expand Down Expand Up @@ -2194,6 +2196,7 @@ void CodeGenFunction::EmitSwitchStmt(const SwitchStmt &S) {
for (unsigned i = 0, e = CaseStmts.size(); i != e; ++i)
EmitStmt(CaseStmts[i]);
incrementProfileCounter(&S);
PGO.markStmtMaybeUsed(S.getBody());

// Now we want to restore the saved switch instance so that nested
// switches continue to function properly
Expand Down
3 changes: 3 additions & 0 deletions clang/lib/CodeGen/CodeGenFunction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1616,6 +1616,8 @@ void CodeGenFunction::GenerateCode(GlobalDecl GD, llvm::Function *Fn,
// Emit the standard function epilogue.
FinishFunction(BodyRange.getEnd());

PGO.verifyCounterMap();

// If we haven't marked the function nothrow through other means, do
// a quick pass now to see if we can.
if (!CurFn->doesNotThrow())
Expand Down Expand Up @@ -1738,6 +1740,7 @@ bool CodeGenFunction::ConstantFoldsToSimpleInteger(const Expr *Cond,
if (!AllowLabels && CodeGenFunction::ContainsLabel(Cond))
return false; // Contains a label.

PGO.markStmtMaybeUsed(Cond);
ResultInt = Int;
return true;
}
Expand Down
26 changes: 25 additions & 1 deletion clang/lib/CodeGen/CodeGenFunction.h
Original file line number Diff line number Diff line change
Expand Up @@ -1620,14 +1620,38 @@ class CodeGenFunction : public CodeGenTypeCache {
uint64_t LoopCount) const;

public:
auto getIsCounterPair(const Stmt *S) const { return PGO.getIsCounterPair(S); }

void markStmtAsUsed(bool Skipped, const Stmt *S) {
PGO.markStmtAsUsed(Skipped, S);
}
void markStmtMaybeUsed(const Stmt *S) { PGO.markStmtMaybeUsed(S); }

enum CounterForIncrement {
Copy link

Choose a reason for hiding this comment

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

doxygen comment would be useful here

Copy link

Choose a reason for hiding this comment

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

is the meaning of "exec counter" and "skip counter" defined anywhere? if not, it would be good to do that here.

UseExecPath = 0,
UseSkipPath,
};

/// 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(UseExecPath, S, false, StepV);
}

/// Emit increment of Counter.
/// \param ExecSkip Use `Skipped` Counter if UseSkipPath is specified.
/// \param S The Stmt that Counter is associated.
/// \param UseBoth Mark both Exec/Skip as used. (for verification)
/// \param StepV The offset Value for adding to Counter.
void incrementProfileCounter(CounterForIncrement ExecSkip, 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, (ExecSkip == UseSkipPath),
UseBoth, StepV);
}
PGO.setCurrentStmt(S);
}
Expand Down
44 changes: 44 additions & 0 deletions clang/lib/CodeGen/CodeGenModule.h
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,50 @@ enum ForDefinition_t : bool {
ForDefinition = true
};

/// The Counter with an optional additional Counter for
/// branches. `Skipped` counter can be calculated with `Executed` and
/// a common Counter (like `Parent`) as `(Parent-Executed)`.
///
/// In SingleByte mode, Counters are binary. Subtraction is not
/// applicable (but addition is capable). In this case, both
/// `Executed` and `Skipped` counters are required. `Skipped` is
/// `None` by default. It is allocated in the coverage mapping.
///
/// There might be cases that `Parent` could be induced with
/// `(Executed+Skipped)`. This is not always applicable.
class CounterPair {
public:
/// Optional value.
class ValueOpt {
private:
static constexpr uint32_t None = (1u << 31); /// None is allocated.
static constexpr uint32_t Mask = None - 1;

uint32_t Val;

public:
ValueOpt() : Val(None) {}

ValueOpt(unsigned InitVal) {
assert(!(InitVal & ~Mask));
Val = InitVal;
}

bool hasValue() const { return !(Val & None); }

operator uint32_t() const { return Val; }
};

ValueOpt Executed;
ValueOpt Skipped; /// May be None.

/// Initialized with Skipped=None.
CounterPair(unsigned Val) : Executed(Val) {}

// FIXME: Should work with {None, None}
CounterPair() : Executed(0) {}
};

struct OrderGlobalInitsOrStermFinalizers {
unsigned int priority;
unsigned int lex_order;
Expand Down
45 changes: 40 additions & 5 deletions clang/lib/CodeGen/CodeGenPGO.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ struct MapRegionCounters : public RecursiveASTVisitor<MapRegionCounters> {
/// The function hash.
PGOHash Hash;
/// The map of statements to counters.
llvm::DenseMap<const Stmt *, unsigned> &CounterMap;
llvm::DenseMap<const Stmt *, CounterPair> &CounterMap;
/// The state of MC/DC Coverage in this function.
MCDC::State &MCDCState;
/// Maximum number of supported MC/DC conditions in a boolean expression.
Expand All @@ -174,7 +174,7 @@ struct MapRegionCounters : public RecursiveASTVisitor<MapRegionCounters> {
DiagnosticsEngine &Diag;

MapRegionCounters(PGOHashVersion HashVersion, uint64_t ProfileVersion,
llvm::DenseMap<const Stmt *, unsigned> &CounterMap,
llvm::DenseMap<const Stmt *, CounterPair> &CounterMap,
MCDC::State &MCDCState, unsigned MCDCMaxCond,
DiagnosticsEngine &Diag)
: NextCounter(0), Hash(HashVersion), CounterMap(CounterMap),
Expand Down Expand Up @@ -1083,7 +1083,7 @@ void CodeGenPGO::mapRegionCounters(const Decl *D) {
(CGM.getCodeGenOpts().MCDCCoverage ? CGM.getCodeGenOpts().MCDCMaxConds
: 0);

RegionCounterMap.reset(new llvm::DenseMap<const Stmt *, unsigned>);
RegionCounterMap.reset(new llvm::DenseMap<const Stmt *, CounterPair>);
RegionMCDCState.reset(new MCDC::State);
MapRegionCounters Walker(HashVersion, ProfileVersion, *RegionCounterMap,
*RegionMCDCState, MCDCMaxConditions, CGM.getDiags());
Expand Down Expand Up @@ -1137,6 +1137,16 @@ void CodeGenPGO::emitCounterRegionMapping(const Decl *D) {
if (CoverageMapping.empty())
return;

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

CGM.getCoverageMapping()->addFunctionMappingRecord(
FuncNameVar, FuncName, FunctionHash, CoverageMapping);
}
Expand Down Expand Up @@ -1185,12 +1195,37 @@ CodeGenPGO::applyFunctionAttributes(llvm::IndexedInstrProfReader *PGOReader,
Fn->setEntryCount(FunctionCount);
}

std::pair<bool, bool> CodeGenPGO::getIsCounterPair(const Stmt *S) const {
if (!RegionCounterMap)
return {false, false};

auto I = RegionCounterMap->find(S);
if (I == RegionCounterMap->end())
return {false, false};

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

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];
unsigned Counter;
auto &TheMap = (*RegionCounterMap)[S];
if (!UseSkipPath) {
if (!TheMap.Executed.hasValue())
return;
Counter = TheMap.Executed;
} else {
if (!TheMap.Skipped.hasValue())
return;
Counter = TheMap.Skipped;
}

if (!Builder.GetInsertBlock())
return;

// Make sure that pointer to global is passed in with zero addrspace
// This is relevant during GPU profiling
Expand Down
18 changes: 16 additions & 2 deletions clang/lib/CodeGen/CodeGenPGO.h
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class CodeGenPGO {
std::array <unsigned, llvm::IPVK_Last + 1> NumValueSites;
unsigned NumRegionCounters;
uint64_t FunctionHash;
std::unique_ptr<llvm::DenseMap<const Stmt *, unsigned>> RegionCounterMap;
std::unique_ptr<llvm::DenseMap<const Stmt *, CounterPair>> RegionCounterMap;
std::unique_ptr<llvm::DenseMap<const Stmt *, uint64_t>> StmtCountMap;
std::unique_ptr<llvm::InstrProfRecord> ProfRecord;
std::unique_ptr<MCDC::State> RegionMCDCState;
Expand Down Expand Up @@ -110,7 +110,9 @@ class CodeGenPGO {
bool canEmitMCDCCoverage(const CGBuilderTy &Builder);

public:
std::pair<bool, bool> getIsCounterPair(const Stmt *S) const;
void emitCounterSetOrIncrement(CGBuilderTy &Builder, const Stmt *S,
bool UseFalsePath, bool UseBoth,
llvm::Value *StepV);
void emitMCDCTestVectorBitmapUpdate(CGBuilderTy &Builder, const Expr *S,
Address MCDCCondBitmapAddr,
Expand All @@ -122,6 +124,18 @@ class CodeGenPGO {
Address MCDCCondBitmapAddr, llvm::Value *Val,
CodeGenFunction &CGF);

void markStmtAsUsed(bool Skipped, const Stmt *S) {
// Do nothing.
}

void markStmtMaybeUsed(const Stmt *S) {
// Do nothing.
}

void verifyCounterMap() const {
// Do nothing.
}

/// Return the region count for the counter at the given index.
uint64_t getRegionCount(const Stmt *S) {
if (!RegionCounterMap)
Expand All @@ -130,7 +144,7 @@ class CodeGenPGO {
return 0;
// With profiles from a differing version of clang we can have mismatched
// decl counts. Don't crash in such a case.
auto Index = (*RegionCounterMap)[S];
auto Index = (*RegionCounterMap)[S].Executed;
if (Index >= RegionCounts.size())
return 0;
return RegionCounts[Index];
Expand Down
Loading
Loading