[clang] [Coverage] Introduce the type `CounterPair` for RegionCounterMap. NFC. (PR #112724)
NAKAMURA Takumi via cfe-commits
cfe-commits at lists.llvm.org
Thu Oct 24 07:03:27 PDT 2024
https://github.com/chapuni updated https://github.com/llvm/llvm-project/pull/112724
>From e4172ca273a6fdfcbfc4662c9e37276ef34c2df4 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4civic at gmail.com>
Date: Thu, 17 Oct 2024 00:32:26 +0900
Subject: [PATCH 1/3] Introduce the type `CounterPair` for RegionCounterMap
`CounterPair` can hold `<uint32_t, uint32_t>` instead of current
`unsigned`, to hold also the counter number of SkipPath. For now, this
change provides the skeleton and only `CounterPair::first` is used.
Each counter number can have `None` to suppress emitting counter
increment. `second` is initialized as `None` by default, since most
`Stmt*` don't have a pair of counters.
This change also provides stubs for the verifyer. I'll provide the
impl of verifier for `+Asserts` later.
`markStmtAsUsed(bool, Stmt*)` may be used to inform that other side
counter may not emitted.
`markStmtMaybeUsed(S)` may be used for the `Stmt` and its inner will
be excluded for emission in the case of skipping by constant
folding. I put it into places where I found.
`verifyCounterMap()` will check the coverage map the counter map and
can be used to report inconsistency.
These verifier methods shall be eliminated in `-Asserts`.
https://discourse.llvm.org/t/rfc-integrating-singlebytecoverage-with-branch-coverage/82492
---
clang/lib/CodeGen/CGDecl.cpp | 9 ++++++++-
clang/lib/CodeGen/CGExpr.cpp | 1 +
clang/lib/CodeGen/CGExprScalar.cpp | 9 +++++++--
clang/lib/CodeGen/CGStmt.cpp | 3 +++
clang/lib/CodeGen/CodeGenFunction.cpp | 3 +++
clang/lib/CodeGen/CodeGenFunction.h | 6 ++++++
clang/lib/CodeGen/CodeGenModule.h | 19 +++++++++++++++++++
clang/lib/CodeGen/CodeGenPGO.cpp | 14 ++++++++++----
clang/lib/CodeGen/CodeGenPGO.h | 17 +++++++++++++++--
clang/lib/CodeGen/CoverageMappingGen.cpp | 6 +++---
clang/lib/CodeGen/CoverageMappingGen.h | 5 +++--
11 files changed, 78 insertions(+), 14 deletions(-)
diff --git a/clang/lib/CodeGen/CGDecl.cpp b/clang/lib/CodeGen/CGDecl.cpp
index 563f728e29d781..ed5f41b624b62b 100644
--- a/clang/lib/CodeGen/CGDecl.cpp
+++ b/clang/lib/CodeGen/CGDecl.cpp
@@ -362,6 +362,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());
@@ -1869,7 +1871,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();
}
@@ -1978,6 +1983,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);
diff --git a/clang/lib/CodeGen/CGExpr.cpp b/clang/lib/CodeGen/CGExpr.cpp
index 52d2f6d52abf94..2fd6b02a3395ee 100644
--- a/clang/lib/CodeGen/CGExpr.cpp
+++ b/clang/lib/CodeGen/CGExpr.cpp
@@ -5134,6 +5134,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())) {
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index b7f5b932c56b6f..74e93f889f4261 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -4982,8 +4982,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.
@@ -5122,8 +5124,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.
@@ -5247,6 +5251,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
diff --git a/clang/lib/CodeGen/CGStmt.cpp b/clang/lib/CodeGen/CGStmt.cpp
index 41dc91c578c800..dbc1ce9bf993cd 100644
--- a/clang/lib/CodeGen/CGStmt.cpp
+++ b/clang/lib/CodeGen/CGStmt.cpp
@@ -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;
}
@@ -845,6 +846,7 @@ void CodeGenFunction::EmitIfStmt(const IfStmt &S) {
RunCleanupsScope ExecutedScope(*this);
EmitStmt(Executed);
}
+ PGO.markStmtMaybeUsed(Skipped);
return;
}
}
@@ -2170,6 +2172,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
diff --git a/clang/lib/CodeGen/CodeGenFunction.cpp b/clang/lib/CodeGen/CodeGenFunction.cpp
index 24723e392c2a3a..371aa494e014bd 100644
--- a/clang/lib/CodeGen/CodeGenFunction.cpp
+++ b/clang/lib/CodeGen/CodeGenFunction.cpp
@@ -1606,6 +1606,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())
@@ -1728,6 +1730,7 @@ bool CodeGenFunction::ConstantFoldsToSimpleInteger(const Expr *Cond,
if (!AllowLabels && CodeGenFunction::ContainsLabel(Cond))
return false; // Contains a label.
+ PGO.markStmtMaybeUsed(Cond);
ResultInt = Int;
return true;
}
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index 9ba0ed02a564dd..89ac3b342d0a7c 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -1620,6 +1620,12 @@ class CodeGenFunction : public CodeGenTypeCache {
uint64_t LoopCount) const;
public:
+ std::pair<bool, bool> getIsCounterPair(const Stmt *S) const {
+ return PGO.getIsCounterPair(S);
+ }
+
+ void markStmtMaybeUsed(const Stmt *S) { PGO.markStmtMaybeUsed(S); }
+
/// 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) {
diff --git a/clang/lib/CodeGen/CodeGenModule.h b/clang/lib/CodeGen/CodeGenModule.h
index c58bb88035ca8a..9dc497321b42a6 100644
--- a/clang/lib/CodeGen/CodeGenModule.h
+++ b/clang/lib/CodeGen/CodeGenModule.h
@@ -101,6 +101,25 @@ enum ForDefinition_t : bool {
ForDefinition = true
};
+class CounterPair : public std::pair<uint32_t, uint32_t> {
+private:
+ static constexpr uint32_t None = (1u << 31); /// None is set
+
+public:
+ static constexpr uint32_t Mask = None - 1;
+
+public:
+ CounterPair(unsigned Val = 0) {
+ assert(!(Val & ~Mask));
+ first = Val;
+ second = None;
+ }
+
+ std::pair<bool, bool> getIsCounterPair() const {
+ return {!(first & None), !(second & None)};
+ }
+};
+
struct OrderGlobalInitsOrStermFinalizers {
unsigned int priority;
unsigned int lex_order;
diff --git a/clang/lib/CodeGen/CodeGenPGO.cpp b/clang/lib/CodeGen/CodeGenPGO.cpp
index 820bb521ccf850..069469e3de856b 100644
--- a/clang/lib/CodeGen/CodeGenPGO.cpp
+++ b/clang/lib/CodeGen/CodeGenPGO.cpp
@@ -164,7 +164,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.
@@ -175,7 +175,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),
@@ -1084,7 +1084,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());
@@ -1186,12 +1186,18 @@ CodeGenPGO::applyFunctionAttributes(llvm::IndexedInstrProfReader *PGOReader,
Fn->setEntryCount(FunctionCount);
}
+std::pair<bool, bool> CodeGenPGO::getIsCounterPair(const Stmt *S) const {
+ if (!RegionCounterMap || RegionCounterMap->count(S) == 0)
+ return {false, false};
+ return (*RegionCounterMap)[S].getIsCounterPair();
+}
+
void CodeGenPGO::emitCounterSetOrIncrement(CGBuilderTy &Builder, const Stmt *S,
llvm::Value *StepV) {
if (!RegionCounterMap || !Builder.GetInsertBlock())
return;
- unsigned Counter = (*RegionCounterMap)[S];
+ unsigned Counter = (*RegionCounterMap)[S].first;
// Make sure that pointer to global is passed in with zero addrspace
// This is relevant during GPU profiling
diff --git a/clang/lib/CodeGen/CodeGenPGO.h b/clang/lib/CodeGen/CodeGenPGO.h
index 9d66ffad6f4350..83f35785e5327d 100644
--- a/clang/lib/CodeGen/CodeGenPGO.h
+++ b/clang/lib/CodeGen/CodeGenPGO.h
@@ -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;
@@ -110,6 +110,7 @@ class CodeGenPGO {
bool canEmitMCDCCoverage(const CGBuilderTy &Builder);
public:
+ std::pair<bool, bool> getIsCounterPair(const Stmt *S) const;
void emitCounterSetOrIncrement(CGBuilderTy &Builder, const Stmt *S,
llvm::Value *StepV);
void emitMCDCTestVectorBitmapUpdate(CGBuilderTy &Builder, const Expr *S,
@@ -122,6 +123,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() {
+ // Do nothing.
+ }
+
/// Return the region count for the counter at the given index.
uint64_t getRegionCount(const Stmt *S) {
if (!RegionCounterMap)
@@ -130,7 +143,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].first;
if (Index >= RegionCounts.size())
return 0;
return RegionCounts[Index];
diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 07015834bc84f3..08e4ac80e0e87c 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -885,7 +885,7 @@ struct CounterCoverageMappingBuilder
: public CoverageMappingBuilder,
public ConstStmtVisitor<CounterCoverageMappingBuilder> {
/// The map of statements to count values.
- llvm::DenseMap<const Stmt *, unsigned> &CounterMap;
+ llvm::DenseMap<const Stmt *, CounterPair> &CounterMap;
MCDC::State &MCDCState;
@@ -938,7 +938,7 @@ struct CounterCoverageMappingBuilder
///
/// This should only be called on statements that have a dedicated counter.
Counter getRegionCounter(const Stmt *S) {
- return Counter::getCounter(CounterMap[S]);
+ return Counter::getCounter(CounterMap[S].first);
}
/// Push a region onto the stack.
@@ -1421,7 +1421,7 @@ struct CounterCoverageMappingBuilder
CounterCoverageMappingBuilder(
CoverageMappingModuleGen &CVM,
- llvm::DenseMap<const Stmt *, unsigned> &CounterMap,
+ 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) {}
diff --git a/clang/lib/CodeGen/CoverageMappingGen.h b/clang/lib/CodeGen/CoverageMappingGen.h
index fe4b93f3af8561..0ed50597e1dc3e 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.h
+++ b/clang/lib/CodeGen/CoverageMappingGen.h
@@ -95,6 +95,7 @@ class CoverageSourceInfo : public PPCallbacks,
namespace CodeGen {
class CodeGenModule;
+class CounterPair;
namespace MCDC {
struct State;
@@ -158,7 +159,7 @@ class CoverageMappingGen {
CoverageMappingModuleGen &CVM;
SourceManager &SM;
const LangOptions &LangOpts;
- llvm::DenseMap<const Stmt *, unsigned> *CounterMap;
+ llvm::DenseMap<const Stmt *, CounterPair> *CounterMap;
MCDC::State *MCDCState;
public:
@@ -169,7 +170,7 @@ class CoverageMappingGen {
CoverageMappingGen(CoverageMappingModuleGen &CVM, SourceManager &SM,
const LangOptions &LangOpts,
- llvm::DenseMap<const Stmt *, unsigned> *CounterMap,
+ llvm::DenseMap<const Stmt *, CounterPair> *CounterMap,
MCDC::State *MCDCState)
: CVM(CVM), SM(SM), LangOpts(LangOpts), CounterMap(CounterMap),
MCDCState(MCDCState) {}
>From 03cfce188cb45adbe78f7e77c2fdd244650f5a3c Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4civic at gmail.com>
Date: Wed, 23 Oct 2024 14:39:35 +0000
Subject: [PATCH 2/3] CGF::markStmtAsUsed
---
clang/lib/CodeGen/CodeGenFunction.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/clang/lib/CodeGen/CodeGenFunction.h b/clang/lib/CodeGen/CodeGenFunction.h
index 89ac3b342d0a7c..fcad1cbcae5e3d 100644
--- a/clang/lib/CodeGen/CodeGenFunction.h
+++ b/clang/lib/CodeGen/CodeGenFunction.h
@@ -1624,6 +1624,9 @@ class CodeGenFunction : public CodeGenTypeCache {
return PGO.getIsCounterPair(S);
}
+ void markStmtAsUsed(bool Skipped, const Stmt *S) {
+ PGO.markStmtAsUsed(Skipped, S);
+ }
void markStmtMaybeUsed(const Stmt *S) { PGO.markStmtMaybeUsed(S); }
/// Increment the profiler's counter for the given statement by \p StepV.
>From afc8481f7cf20da7de4e95a60bf3ccdb04bd08f3 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4civic at gmail.com>
Date: Wed, 23 Oct 2024 14:39:35 +0000
Subject: [PATCH 3/3] CGF.markStmtMaybeUsed for binop
---
clang/lib/CodeGen/CGExprScalar.cpp | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/clang/lib/CodeGen/CGExprScalar.cpp b/clang/lib/CodeGen/CGExprScalar.cpp
index 74e93f889f4261..9e8533ca90a5cd 100644
--- a/clang/lib/CodeGen/CGExprScalar.cpp
+++ b/clang/lib/CodeGen/CGExprScalar.cpp
@@ -4970,7 +4970,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.
@@ -5112,7 +5113,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.
More information about the cfe-commits
mailing list