Skip to content
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

[Coverage] Let Decision take account of expansions #78969

Merged
merged 12 commits into from
Feb 2, 2024
130 changes: 115 additions & 15 deletions llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include "llvm/ProfileData/Coverage/CoverageMapping.h"
#include "llvm/ADT/ArrayRef.h"
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallBitVector.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringExtras.h"
Expand Down Expand Up @@ -582,6 +583,87 @@ static unsigned getMaxBitmapSize(const CounterMappingContext &Ctx,
return MaxBitmapID + (SizeInBits / CHAR_BIT);
}

/// This holds the DecisionRegion and MCDCBranch(es) under it.
/// Also traverses Expansion(s).
struct DecisionRow {
chapuni marked this conversation as resolved.
Show resolved Hide resolved
/// The subject
chapuni marked this conversation as resolved.
Show resolved Hide resolved
const CounterMappingRegion *DecisionRegion;

/// They are reflected from `DecisionRegion` for convenience.
LineColPair DecisionStartLoc;
LineColPair DecisionEndLoc;

/// This is passed to `MCDCRecordProcessor`, so this should be compatible to
/// `ArrayRef<const CounterMappingRegion *>`.
SmallVector<const CounterMappingRegion *, 6> Branches;
chapuni marked this conversation as resolved.
Show resolved Hide resolved

/// Each `ID` in `Branches` should be unique.
chapuni marked this conversation as resolved.
Show resolved Hide resolved
DenseSet<CounterMappingRegion::MCDCConditionID> IDs;

/// Relevant `Expansion`(s) should be caught to find expanded Branches.
SmallVector<const CounterMappingRegion *> Expansions;

DecisionRow(const CounterMappingRegion &Decision)
: DecisionRegion(&Decision), DecisionStartLoc(Decision.startLoc()),
DecisionEndLoc(Decision.endLoc()) {}

/// Determine whether `R` is included in `DecisionRegion`.
Copy link

Choose a reason for hiding this comment

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

Nit:

E.g.

\returns true if \p R is included in DecisionRegion

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 haven't written doxygen comments for long time. Please point out any mistakes.

Copy link
Member

Choose a reason for hiding this comment

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

Doxygen comments are not required. I think much new code doesn't use Doxygen...

bool inDecisionRegion(const CounterMappingRegion &R) {
return (R.FileID == DecisionRegion->FileID &&
R.startLoc() >= DecisionStartLoc && R.endLoc() <= DecisionEndLoc);
}

/// Determin whether `R` is pointed by any of Expansions.
chapuni marked this conversation as resolved.
Show resolved Hide resolved
bool inExpansions(const CounterMappingRegion &R) {
return any_of(Expansions, [&](const auto &Expansion) {
return (Expansion->ExpandedFileID == R.FileID);
});
}

enum class UpdateResult {
Copy link

Choose a reason for hiding this comment

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

This should be documented

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 didn't do since I thought the enums are descriptive.

NotFound = 0,
Updated,
Completed,
};

/// Add `Branch` into the Decision
UpdateResult updateBranch(const CounterMappingRegion &Branch) {
chapuni marked this conversation as resolved.
Show resolved Hide resolved
auto ID = Branch.MCDCParams.ID;
assert(ID > 0 && "MCDCBranch.ID should begin with 1");

if (!IDs.contains(ID) &&
chapuni marked this conversation as resolved.
Show resolved Hide resolved
(inDecisionRegion(Branch) || inExpansions(Branch))) {
assert(Branches.size() < DecisionRegion->MCDCParams.NumConditions);

// Put `ID=1` in front of `Branches` for convenience
// even if `Branches` is not topological.
if (ID == 1)
Branches.insert(Branches.begin(), &Branch);
else
Branches.push_back(&Branch);

// Mark `ID` as `assigned`.
IDs.insert(ID);

// `Completed` when `Branches` is full
return (Branches.size() == DecisionRegion->MCDCParams.NumConditions
? UpdateResult::Completed
: UpdateResult::Updated);
} else
return UpdateResult::NotFound;
}

/// Record `Expansion` if it is dominated to the Decision.
/// Each `Expansion` may nest.
bool updateExpansion(const CounterMappingRegion &Expansion) {
chapuni marked this conversation as resolved.
Show resolved Hide resolved
if (inDecisionRegion(Expansion) || inExpansions(Expansion)) {
Expansions.push_back(&Expansion);
return true;
} else
chapuni marked this conversation as resolved.
Show resolved Hide resolved
return false;
}
};

Error CoverageMapping::loadFunctionRecord(
const CoverageMappingRecord &Record,
IndexedInstrProfReader &ProfileReader) {
Expand Down Expand Up @@ -638,18 +720,12 @@ Error CoverageMapping::loadFunctionRecord(
Record.MappingRegions[0].Count.isZero() && Counts[0] > 0)
return Error::success();

unsigned NumConds = 0;
const CounterMappingRegion *MCDCDecision;
std::vector<const CounterMappingRegion *> MCDCBranches;

SmallVector<DecisionRow> Decisions;
FunctionRecord Function(OrigFuncName, Record.Filenames);
for (const auto &Region : Record.MappingRegions) {
// If an MCDCDecisionRegion is seen, track the BranchRegions that follow
// it according to Region.NumConditions.
if (Region.Kind == CounterMappingRegion::MCDCDecisionRegion) {
assert(NumConds == 0);
MCDCDecision = &Region;
NumConds = Region.MCDCParams.NumConditions;
// Start recording `Region` as the `Decision`
Decisions.emplace_back(Region);
Copy link

Choose a reason for hiding this comment

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

why emplace_back instead of push_back?

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 thought it would be made smaller if the constructor has default constructors for SmallVector.

continue;
}
Expected<int64_t> ExecutionCount = Ctx.evaluate(Region.Count);
Expand All @@ -664,23 +740,39 @@ Error CoverageMapping::loadFunctionRecord(
}
Function.pushRegion(Region, *ExecutionCount, *AltExecutionCount);

if (Region.Kind == CounterMappingRegion::ExpansionRegion) {
for (auto &Decision : reverse(Decisions)) {
chapuni marked this conversation as resolved.
Show resolved Hide resolved
if (Decision.updateExpansion(Region))
break;
}
continue;
}

if (Region.Kind != CounterMappingRegion::MCDCBranchRegion)
continue;

// If a MCDCDecisionRegion was seen, store the BranchRegions that
// correspond to it in a vector, according to the number of conditions
// recorded for the region (tracked by NumConds).
if (NumConds > 0 && Region.Kind == CounterMappingRegion::MCDCBranchRegion) {
MCDCBranches.push_back(&Region);
for (int I = Decisions.size() - 1; I >= 0; --I) {
auto &Decision = Decisions[I];

// As we move through all of the MCDCBranchRegions that follow the
chapuni marked this conversation as resolved.
Show resolved Hide resolved
// MCDCDecisionRegion, decrement NumConds to make sure we account for
// them all before we calculate the bitmap of executed test vectors.
if (--NumConds == 0) {
switch (Decision.updateBranch(Region)) {
case DecisionRow::UpdateResult::NotFound:
continue;
case DecisionRow::UpdateResult::Updated:
goto branch_found;
chapuni marked this conversation as resolved.
Show resolved Hide resolved
case DecisionRow::UpdateResult::Completed:
// Evaluating the test vector bitmap for the decision region entails
// calculating precisely what bits are pertinent to this region alone.
// This is calculated based on the recorded offset into the global
// profile bitmap; the length is calculated based on the recorded
// number of conditions.
Expected<BitVector> ExecutedTestVectorBitmap =
Ctx.evaluateBitmap(MCDCDecision);
Ctx.evaluateBitmap(Decision.DecisionRegion);
if (auto E = ExecutedTestVectorBitmap.takeError()) {
consumeError(std::move(E));
return Error::success();
Expand All @@ -690,19 +782,27 @@ Error CoverageMapping::loadFunctionRecord(
// DecisionRegion, all of the information is now available to process.
// This is where the bulk of the MC/DC progressing takes place.
Expected<MCDCRecord> Record = Ctx.evaluateMCDCRegion(
*MCDCDecision, *ExecutedTestVectorBitmap, MCDCBranches);
*Decision.DecisionRegion, *ExecutedTestVectorBitmap,
Decision.Branches);
if (auto E = Record.takeError()) {
consumeError(std::move(E));
return Error::success();
}

// Save the MC/DC Record so that it can be visualized later.
Function.pushMCDCRecord(*Record);
MCDCBranches.clear();

Decisions.erase(Decisions.begin() + I);
goto branch_found;
}
}
llvm_unreachable("Branch not found in Decisions");

branch_found:;
}

assert(Decisions.empty() && "All Decisions have not been resolved");

// Don't create records for (filenames, function) pairs we've already seen.
auto FilenamesHash = hash_combine_range(Record.Filenames.begin(),
Record.Filenames.end());
Expand Down