[llvm-branch-commits] [clang] [MC/DC] Refactor MCDCCoverageBuilder. NFC. (PR #125409)
NAKAMURA Takumi via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Sun Feb 2 05:32:44 PST 2025
https://github.com/chapuni created https://github.com/llvm/llvm-project/pull/125409
- Get rid of the old `DecisionStack` and dissolve it into push/pop `CurCondIDs` in `VisitBin`, since `VisitBin` is recursive.
- Introduce the new `DecisionStack` with `DecisionState` to handle the current `Decision` in nested `Decision`s.
- The stack has the sentinel that has `DecisionExpr = nullptr`.
- Split out `checkDecisionRootOrPush` from `pushAndAssignIDs` for non-BinOp. It assigns `CondID` to `E` (instead of assignment LHS in `pushAndAssignIDs`).
- The stack is manupilated at the top Decision operator in `VisitBin`.
- The stack grows at the entrance of the Decision with the initial state.
- In the same level in `VisitBin`, the stack is popped and the `Decision` record is emitted.
- Introduce `DecisionEndToSince` to sweep `MCDCBranch`es partially in `cancelDecision`.
>From 2b37ea400809fd57f2b71b997d5dca622113a422 Mon Sep 17 00:00:00 2001
From: NAKAMURA Takumi <geek4civic at gmail.com>
Date: Sun, 2 Feb 2025 22:00:22 +0900
Subject: [PATCH] [MC/DC] Refactor MCDCCoverageBuilder. NFC.
- Get rid of the old `DecisionStack` and dissolve it into push/pop
`CurCondIDs` in `VisitBin`, since `VisitBin` is recursive.
- Introduce the new `DecisionStack` with `DecisionState` to handle the
current `Decision` in nested `Decision`s.
- The stack has the sentinel that has `DecisionExpr = nullptr`.
- Split out `checkDecisionRootOrPush` from `pushAndAssignIDs` for
non-BinOp. It assigns `CondID` to `E` (instead of assignment LHS
in `pushAndAssignIDs`).
- The stack is manupilated at the top Decision operator in `VisitBin`.
- The stack grows at the entrance of the Decision with the initial
state.
- In the same level in `VisitBin`, the stack is popped and the
`Decision` record is emitted.
- Introduce `DecisionEndToSince` to sweep `MCDCBranch`es partially in
`cancelDecision`.
---
clang/lib/CodeGen/CoverageMappingGen.cpp | 309 ++++++++++++++---------
1 file changed, 187 insertions(+), 122 deletions(-)
diff --git a/clang/lib/CodeGen/CoverageMappingGen.cpp b/clang/lib/CodeGen/CoverageMappingGen.cpp
index 4dbc0c70e34d60..3a281fd39b4bcb 100644
--- a/clang/lib/CodeGen/CoverageMappingGen.cpp
+++ b/clang/lib/CodeGen/CoverageMappingGen.cpp
@@ -750,41 +750,48 @@ struct MCDCCoverageBuilder {
private:
CodeGenModule &CGM;
-
- llvm::SmallVector<mcdc::ConditionIDs> DecisionStack;
MCDC::State &MCDCState;
- const Stmt *DecisionStmt = nullptr;
- mcdc::ConditionID NextID = 0;
- bool NotMapped = false;
- /// Represent a sentinel value as a pair of final decisions for the bottom
- // of DecisionStack.
- static constexpr mcdc::ConditionIDs DecisionStackSentinel{-1, -1};
+ struct DecisionState {
+ /// The root Decision
+ const Expr *DecisionExpr = nullptr;
- /// Is this a logical-AND operation?
- bool isLAnd(const BinaryOperator *E) const {
- return E->getOpcode() == BO_LAnd;
- }
+ /// Pair of Destination conditions [false, true]
+ /// -1, the final decision at the initial state.
+ /// Modify before/after the traversal of BinOp LHS.
+ mcdc::ConditionIDs CurCondIDs = {-1, -1};
+
+ /// The ID to be assigned, and total number of conditions.
+ mcdc::ConditionID NextID = 0;
+
+ /// false if the Decision is recognized but should be ignored.
+ bool Active = false;
+
+ DecisionState() = default;
+ DecisionState(const Expr *DecisionExpr, bool Valid)
+ : DecisionExpr(DecisionExpr), Active(Valid) {}
+ };
+
+ /// The bottom [0] is the sentinel.
+ /// - DecisionExpr = nullptr, doesn't match to any Expr(s).
+ /// - Active = false
+ llvm::SmallVector<DecisionState, 2> DecisionStack;
+
+ /// <Index of Decision, Index of Since>, on SourceRegions.
+ /// Used for restoring MCDCBranch=>Branch.
+ llvm::DenseMap<unsigned, unsigned> DecisionEndToSince;
public:
MCDCCoverageBuilder(CodeGenModule &CGM, MCDC::State &MCDCState)
- : CGM(CGM), DecisionStack(1, DecisionStackSentinel),
- MCDCState(MCDCState) {}
-
- /// Return whether the build of the control flow map is at the top-level
- /// (root) of a logical operator nest in a boolean expression prior to the
- /// assignment of condition IDs.
- bool isIdle() const { return (NextID == 0 && !NotMapped); }
+ : CGM(CGM), MCDCState(MCDCState), DecisionStack(1) {}
- /// Return whether any IDs have been assigned in the build of the control
- /// flow map, indicating that the map is being generated for this boolean
- /// expression.
- bool isBuilding() const { return (NextID > 0); }
+ bool isActive() const { return DecisionStack.back().Active; }
/// Set the given condition's ID.
void setCondID(const Expr *Cond, mcdc::ConditionID ID) {
- MCDCState.BranchByStmt[CodeGenFunction::stripCond(Cond)] = {ID,
- DecisionStmt};
+ assert(isActive());
+ MCDCState.BranchByStmt[CodeGenFunction::stripCond(Cond)] = {
+ ID, DecisionStack.back().DecisionExpr};
}
/// Return the ID of a given condition.
@@ -797,83 +804,99 @@ struct MCDCCoverageBuilder {
}
/// Return the LHS Decision ([0,0] if not set).
- const mcdc::ConditionIDs &back() const { return DecisionStack.back(); }
+ auto &getCurCondIDs() { return DecisionStack.back().CurCondIDs; }
- /// Push the binary operator statement to track the nest level and assign IDs
- /// to the operator's LHS and RHS. The RHS may be a larger subtree that is
- /// broken up on successive levels.
- void pushAndAssignIDs(const BinaryOperator *E) {
- if (!CGM.getCodeGenOpts().MCDCCoverage)
+ void swapConds() {
+ if (!isActive())
return;
- // If binary expression is disqualified, don't do mapping.
- if (!isBuilding() &&
- !MCDCState.DecisionByStmt.contains(CodeGenFunction::stripCond(E)))
- NotMapped = true;
+ std::swap(getCurCondIDs()[false], getCurCondIDs()[true]);
+ }
- // Don't go any further if we don't need to map condition IDs.
- if (NotMapped)
+ void checkDecisionRootOrPush(const Expr *E) {
+ // Don't push the new entry unless MC/DC Coverage.
+ if (!CGM.getCodeGenOpts().MCDCCoverage) {
+ assert(!isActive() && "The setinel should tell 'not Active'");
return;
-
- if (NextID == 0) {
- DecisionStmt = E;
- assert(MCDCState.DecisionByStmt.contains(E));
}
- const mcdc::ConditionIDs &ParentDecision = DecisionStack.back();
+ auto *SC = CodeGenFunction::stripCond(E);
+ if (getCondID(SC) >= 0)
+ return;
- // If the operator itself has an assigned ID, this means it represents a
- // larger subtree. In this case, assign that ID to its LHS node. Its RHS
- // will receive a new ID below. Otherwise, assign ID+1 to LHS.
- if (MCDCState.BranchByStmt.contains(CodeGenFunction::stripCond(E)))
- setCondID(E->getLHS(), getCondID(E));
- else
- setCondID(E->getLHS(), NextID++);
+ // Push the new entry at the Decision root.
+ if (auto DI = MCDCState.DecisionByStmt.find(SC);
+ DI != MCDCState.DecisionByStmt.end()) {
+ auto &StackTop = DecisionStack.emplace_back(SC, DI->second.isValid());
- // Assign a ID+1 for the RHS.
- mcdc::ConditionID RHSid = NextID++;
- setCondID(E->getRHS(), RHSid);
+ // The root expr (possibly BinOp) may have 1st ID.
+ // It will be propagated to the most Left hand.
+ if (isActive() && getCondID(SC) < 0)
+ setCondID(SC, StackTop.NextID++);
+ return;
+ }
- // Push the LHS decision IDs onto the DecisionStack.
- if (isLAnd(E))
- DecisionStack.push_back({ParentDecision[false], RHSid});
- else
- DecisionStack.push_back({RHSid, ParentDecision[true]});
+ assert((!isActive() || DecisionStack.back().NextID > 0) &&
+ "Should be Active and after assignments");
}
- /// Pop and return the LHS Decision ([0,0] if not set).
- mcdc::ConditionIDs pop() {
- if (!CGM.getCodeGenOpts().MCDCCoverage || NotMapped)
- return DecisionStackSentinel;
+ /// Push the binary operator statement to track the nest level and assign IDs
+ /// to the operator's LHS and RHS. The RHS may be a larger subtree that is
+ /// broken up on successive levels.
+ std::pair<mcdc::ConditionID, mcdc::ConditionID>
+ pushAndAssignIDs(const BinaryOperator *E) {
+ if (!CGM.getCodeGenOpts().MCDCCoverage)
+ return {-1, -1};
+
+ checkDecisionRootOrPush(E);
+ if (!isActive())
+ return {-1, -1};
+
+ auto &StackTop = DecisionStack.back();
+
+ // LHS inherits the ID from the parent.
+ mcdc::ConditionID LHSid = getCondID(E);
+ assert(LHSid >= 0);
+ setCondID(E->getLHS(), LHSid);
+
+ // Assign a ID+1 for the RHS.
+ mcdc::ConditionID RHSid = StackTop.NextID++;
+ setCondID(E->getRHS(), RHSid);
- assert(DecisionStack.size() > 1);
- return DecisionStack.pop_back_val();
+ return {LHSid, RHSid};
}
- /// Return the total number of conditions and reset the state. The number of
+ /// Return the total number of conditions and rewind the state. The number of
/// conditions is zero if the expression isn't mapped.
- unsigned getTotalConditionsAndReset(const BinaryOperator *E) {
- if (!CGM.getCodeGenOpts().MCDCCoverage)
- return 0;
-
- assert(!isIdle());
- assert(DecisionStack.size() == 1);
+ unsigned getTotalConditionsAndPop(const Expr *E) {
+ auto &StackTop = DecisionStack.back();
- // Reset state if not doing mapping.
- if (NotMapped) {
- NotMapped = false;
- assert(NextID == 0);
+ // Root?
+ if (StackTop.DecisionExpr != E)
return 0;
- }
-
- // Set number of conditions and reset.
- unsigned TotalConds = NextID;
- // Reset ID back to beginning.
- NextID = 0;
+ assert(StackTop.CurCondIDs[false] == -1 &&
+ StackTop.CurCondIDs[true] == -1 &&
+ "The root shouldn't depend on others.");
+ // Set number of conditions and pop.
+ unsigned TotalConds = (StackTop.Active ? StackTop.NextID : 0);
+ DecisionStack.pop_back();
+ assert(!DecisionStack.empty() && "Sentiel?");
return TotalConds;
}
+
+ void addDecisionRegionRange(unsigned Since, unsigned End) {
+ DecisionEndToSince[End] = Since;
+ }
+
+ /// Returns "Since" index corresponding to the arg Idx.
+ unsigned skipSourceRegionIndexForDecisions(unsigned Idx) {
+ auto I = DecisionEndToSince.find(Idx);
+ assert(I != DecisionEndToSince.end());
+ assert(I->second <= Idx);
+ return I->second;
+ }
};
/// A StmtVisitor that creates coverage mapping regions which map
@@ -2169,19 +2192,39 @@ struct CounterCoverageMappingBuilder
createBranchRegion(E->getCond(), TrueCount, FalseCount);
}
- void createOrCancelDecision(const BinaryOperator *E, unsigned Since) {
- unsigned NumConds = MCDCBuilder.getTotalConditionsAndReset(E);
+ inline unsigned findMCDCBranchesInSourceRegion(
+ unsigned Since, std::function<void(SourceMappingRegion &SR)> CB) {
+ unsigned I = SourceRegions.size() - 1;
+ unsigned Count = 0;
+ while (I >= Since) {
+ auto &SR = SourceRegions[I];
+ if (SR.isMCDCDecision()) {
+ // Skip a sub Decision and don't modify records in it.
+ I = MCDCBuilder.skipSourceRegionIndexForDecisions(I);
+ } else if (SR.isMCDCBranch()) {
+ ++Count;
+ CB(SR);
+ }
+
+ if (I-- <= Since)
+ break;
+ }
+
+ return Count;
+ }
+
+ void createOrCancelDecision(const Expr *E, unsigned Since) {
+ auto *SC = CodeGenFunction::stripCond(E);
+ auto NumConds = MCDCBuilder.getTotalConditionsAndPop(SC);
if (NumConds == 0)
return;
// Extract [ID, Conds] to construct the graph.
llvm::SmallVector<mcdc::ConditionIDs> CondIDs(NumConds);
- for (const auto &SR : ArrayRef(SourceRegions).slice(Since)) {
- if (SR.isMCDCBranch()) {
- auto [ID, Conds] = SR.getMCDCBranchParams();
- CondIDs[ID] = Conds;
- }
- }
+ findMCDCBranchesInSourceRegion(Since, [&](const SourceMappingRegion &SR) {
+ auto [ID, Conds] = SR.getMCDCBranchParams();
+ CondIDs[ID] = Conds;
+ });
// Construct the graph and calculate `Indices`.
mcdc::TVIdxBuilder Builder(CondIDs);
@@ -2191,14 +2234,14 @@ struct CounterCoverageMappingBuilder
if (NumTVs > MaxTVs) {
// NumTVs exceeds MaxTVs -- warn and cancel the Decision.
- cancelDecision(E, Since, NumTVs, MaxTVs);
+ cancelDecision(SC, Since, NumTVs, MaxTVs, NumConds);
return;
}
// Update the state for CodeGenPGO
- assert(MCDCState.DecisionByStmt.contains(E));
- MCDCState.DecisionByStmt[E].update(MCDCState.BitmapBits, // Top
- std::move(Builder.Indices));
+ assert(MCDCState.DecisionByStmt.contains(SC));
+ MCDCState.DecisionByStmt[SC].update(MCDCState.BitmapBits, // Top
+ std::move(Builder.Indices));
auto DecisionParams = mcdc::DecisionParameters{
MCDCState.BitmapBits += NumTVs, // Tail
@@ -2207,28 +2250,39 @@ struct CounterCoverageMappingBuilder
// Create MCDC Decision Region.
createDecisionRegion(E, DecisionParams);
+
+ // Memo
+ assert(SourceRegions.back().isMCDCDecision());
+ MCDCBuilder.addDecisionRegionRange(Since, SourceRegions.size() - 1);
}
// Warn and cancel the Decision.
- void cancelDecision(const BinaryOperator *E, unsigned Since, int NumTVs,
- int MaxTVs) {
+ void cancelDecision(const Expr *Decision, unsigned Since, int NumTVs,
+ int MaxTVs, unsigned NumConds) {
auto &Diag = CVM.getCodeGenModule().getDiags();
unsigned DiagID =
Diag.getCustomDiagID(DiagnosticsEngine::Warning,
"unsupported MC/DC boolean expression; "
"number of test vectors (%0) exceeds max (%1). "
"Expression will not be covered");
- Diag.Report(E->getBeginLoc(), DiagID) << NumTVs << MaxTVs;
+ Diag.Report(Decision->getBeginLoc(), DiagID) << NumTVs << MaxTVs;
// Restore MCDCBranch to Branch.
- for (auto &SR : MutableArrayRef(SourceRegions).slice(Since)) {
- assert(!SR.isMCDCDecision() && "Decision shouldn't be seen here");
- if (SR.isMCDCBranch())
- SR.resetMCDCParams();
- }
+ unsigned FoundCount = findMCDCBranchesInSourceRegion(
+ Since, [](SourceMappingRegion &SR) { SR.resetMCDCParams(); });
+ assert(FoundCount == NumConds &&
+ "Didn't find all MCDCBranches to be restored");
+ (void)FoundCount;
// Tell CodeGenPGO not to instrument.
- MCDCState.DecisionByStmt.erase(E);
+ for (auto I = MCDCState.BranchByStmt.begin(),
+ E = MCDCState.BranchByStmt.end();
+ I != E;) {
+ auto II = I++;
+ if (II->second.DecisionStmt == Decision)
+ MCDCState.BranchByStmt.erase(II);
+ }
+ MCDCState.DecisionByStmt.erase(Decision);
}
/// Check if E belongs to system headers.
@@ -2245,19 +2299,29 @@ struct CounterCoverageMappingBuilder
return;
}
- bool IsRootNode = MCDCBuilder.isIdle();
-
unsigned SourceRegionsSince = SourceRegions.size();
// Keep track of Binary Operator and assign MCDC condition IDs.
- MCDCBuilder.pushAndAssignIDs(E);
+ auto [_, RHSid] = MCDCBuilder.pushAndAssignIDs(E);
+
+ // DecisionRHS inherits CurCondIDs.
+ auto &CurCondIDs = MCDCBuilder.getCurCondIDs();
+ auto DecisionRHS = CurCondIDs;
+
+ CurCondIDs[true] = RHSid;
+ auto DecisionLHS = CurCondIDs;
extendRegion(E->getLHS());
propagateCounts(getRegion().getCounter(), E->getLHS());
handleFileExit(getEnd(E->getLHS()));
- // Track LHS True/False Decision.
- const auto DecisionLHS = MCDCBuilder.pop();
+ // Restore CurCondIDs.
+ {
+ auto &CurCondIDs =
+ MCDCBuilder.getCurCondIDs(); // Stack may be reallocated.
+ CurCondIDs[true] = DecisionRHS[true];
+ assert(CurCondIDs == DecisionRHS);
+ }
// Counter tracks the right hand side of a logical and operator.
extendRegion(E->getRHS());
@@ -2266,9 +2330,6 @@ struct CounterCoverageMappingBuilder
if (llvm::EnableSingleByteCoverage)
return;
- // Track RHS True/False Decision.
- const auto DecisionRHS = MCDCBuilder.back();
-
// Extract the Parent Region Counter.
Counter ParentCnt = getRegion().getCounter();
@@ -2285,9 +2346,8 @@ struct CounterCoverageMappingBuilder
// Create Branch Region around RHS condition.
createBranchRegion(E->getRHS(), RHSTrueCnt, RHSExitCnt, DecisionRHS);
- // Create MCDC Decision Region if at top-level (root).
- if (IsRootNode)
- createOrCancelDecision(E, SourceRegionsSince);
+ // Create MCDC Decision Region when E is at the top level.
+ createOrCancelDecision(E, SourceRegionsSince);
}
// Determine whether the right side of OR operation need to be visited.
@@ -2306,19 +2366,28 @@ struct CounterCoverageMappingBuilder
return;
}
- bool IsRootNode = MCDCBuilder.isIdle();
-
unsigned SourceRegionsSince = SourceRegions.size();
// Keep track of Binary Operator and assign MCDC condition IDs.
- MCDCBuilder.pushAndAssignIDs(E);
+ auto [_, RHSid] = MCDCBuilder.pushAndAssignIDs(E);
+
+ // Push the LHS decision IDs onto the DecisionStack.
+ auto &CurCondIDs = MCDCBuilder.getCurCondIDs();
+ auto DecisionRHS = CurCondIDs;
+ CurCondIDs[false] = RHSid;
+ auto DecisionLHS = CurCondIDs;
extendRegion(E->getLHS());
Counter OutCount = propagateCounts(getRegion().getCounter(), E->getLHS());
handleFileExit(getEnd(E->getLHS()));
// Track LHS True/False Decision.
- const auto DecisionLHS = MCDCBuilder.pop();
+ {
+ auto &CurCondIDs =
+ MCDCBuilder.getCurCondIDs(); // Stack may be reallocated.
+ CurCondIDs[false] = DecisionRHS[false];
+ assert(CurCondIDs == DecisionRHS);
+ }
// Counter tracks the right hand side of a logical or operator.
extendRegion(E->getRHS());
@@ -2327,9 +2396,6 @@ struct CounterCoverageMappingBuilder
if (llvm::EnableSingleByteCoverage)
return;
- // Track RHS True/False Decision.
- const auto DecisionRHS = MCDCBuilder.back();
-
// Extract the Parent Region Counter.
Counter ParentCnt = getRegion().getCounter();
@@ -2350,9 +2416,8 @@ struct CounterCoverageMappingBuilder
// Create Branch Region around RHS condition.
createBranchRegion(E->getRHS(), RHSExitCnt, RHSFalseCnt, DecisionRHS);
- // Create MCDC Decision Region if at top-level (root).
- if (IsRootNode)
- createOrCancelDecision(E, SourceRegionsSince);
+ // Create MCDC Decision Region when E is at the top level.
+ createOrCancelDecision(E, SourceRegionsSince);
}
void VisitLambdaExpr(const LambdaExpr *LE) {
More information about the llvm-branch-commits
mailing list