Skip to content

Introduce CounterExpressionBuilder::subst(C, Map) #112698

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 8 commits into from
Jan 9, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
6 changes: 6 additions & 0 deletions llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#include <cassert>
#include <cstdint>
#include <iterator>
#include <map>
#include <memory>
#include <sstream>
#include <string>
Expand Down Expand Up @@ -213,6 +214,11 @@ class CounterExpressionBuilder {
/// Return a counter that represents the expression that subtracts RHS from
/// LHS.
Counter subtract(Counter LHS, Counter RHS, bool Simplify = true);

using ReplaceMap = std::map<Counter, Counter>;

/// Return a counter for each term in the expression replaced by ReplaceMap.
Counter replace(Counter C, const ReplaceMap &Map);
};

using LineColPair = std::pair<unsigned, unsigned>;
Expand Down
32 changes: 32 additions & 0 deletions llvm/lib/ProfileData/Coverage/CoverageMapping.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,38 @@ Counter CounterExpressionBuilder::subtract(Counter LHS, Counter RHS,
return Simplify ? simplify(Cnt) : Cnt;
}

Counter CounterExpressionBuilder::replace(Counter C, const ReplaceMap &Map) {
auto I = Map.find(C);

// Replace C with the Map even if C is Expression.
Copy link

Choose a reason for hiding this comment

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

Replace C with the Map...

"replace C with the value found in Map..."?

if (I != Map.end())
return I->second;

// Traverse only Expression.
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.

Because others (Zero or CounterRef) are not traversable :)

Is this line too naive to you? I could eliminate.

if (!C.isExpression())
return C;

auto CE = Expressions[C.getExpressionID()];
auto NewLHS = replace(CE.LHS, Map);
Copy link

Choose a reason for hiding this comment

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

How deep can this recurse? Should there be a depth limit?

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 suppose at most the number of terms, unless the expression is made buggy with loops. (Then, other CounterExpression stuff would hang or crash, I guess) Would we really check if the expr would not be buggy?

If we should introduce such a check, it may be not depth limit but the set of Seen.

Copy link

Choose a reason for hiding this comment

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

If we don't want a check, is it possible to add debug output?

auto NewRHS = replace(CE.RHS, Map);

// Reconstruct Expression with induced subexpressions.
switch (CE.Kind) {
case CounterExpression::Add:
C = add(NewLHS, NewRHS);
break;
case CounterExpression::Subtract:
C = subtract(NewLHS, NewRHS);
break;
}

// Reconfirm if the reconstructed expression would hit the Map.
if ((I = Map.find(C)) != Map.end())
return I->second;

return C;
}

void CounterMappingContext::dump(const Counter &C, raw_ostream &OS) const {
switch (C.getKind()) {
case Counter::Zero:
Expand Down
Loading