Skip to content

[Coverage] Introduce the type CounterPair for RegionCounterMap. NFC. #112724

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

Merged
merged 7 commits into from
Jan 9, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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);
Copy link

Choose a reason for hiding this comment

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

why?

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 don't remember why I marked here. I was just suppressing checks when I met an issue.

Copy link

Choose a reason for hiding this comment

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

Should it be removed then?

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 should be compiled as an empty body and will be pruned. I will introduce debug facility that is enabled only in +Asserts.

I prioritized implementing the debug facility as low.

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
7 changes: 7 additions & 0 deletions clang/lib/CodeGen/CodeGenFunction.h
Original file line number Diff line number Diff line change
Expand Up @@ -1620,6 +1620,13 @@ 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); }

/// 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) {
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
19 changes: 15 additions & 4 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 @@ -1185,12 +1185,23 @@ 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,
llvm::Value *StepV) {
if (!RegionCounterMap || !Builder.GetInsertBlock())
return;

unsigned Counter = (*RegionCounterMap)[S];
unsigned Counter = (*RegionCounterMap)[S].Executed;

// Make sure that pointer to global is passed in with zero addrspace
// This is relevant during GPU profiling
Expand Down
17 changes: 15 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,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,
Expand All @@ -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() 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 +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].Executed;
if (Index >= RegionCounts.size())
return 0;
return RegionCounts[Index];
Expand Down
6 changes: 3 additions & 3 deletions clang/lib/CodeGen/CoverageMappingGen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -882,7 +882,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;

Expand Down Expand Up @@ -935,7 +935,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].Executed);
}

/// Push a region onto the stack.
Expand Down Expand Up @@ -1417,7 +1417,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) {}
Expand Down
5 changes: 3 additions & 2 deletions clang/lib/CodeGen/CoverageMappingGen.h
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ class CoverageSourceInfo : public PPCallbacks,
namespace CodeGen {

class CodeGenModule;
class CounterPair;

namespace MCDC {
struct State;
Expand Down Expand Up @@ -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:
Expand All @@ -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) {}
Expand Down
Loading