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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
View file
Open in desktop
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>;
Copy link

@ornata ornata Oct 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

why std::map instead of e.g. DenseMap?

https://llvm.org/docs/ProgrammersManual.html#map

std::map has similar characteristics to std::set: it uses a single allocation per pair inserted into the map, it offers log(n) lookup with an extremely large constant factor, imposes a space penalty of 3 pointers per pair in the map, etc.

std::map is most useful when your keys or values are very large, if you need to iterate over the collection in sorted order, or if you need stable iterators into the map (i.e. they don’t get invalidated if an insertion or deletion of another element takes place).

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 am so lazy to add hash helper into llvm::Counter. I have taken std::map since Counter has provided enough operators.

The map may grow up to hundreds of keys. Could we make Counter ready for hash maps?


/// 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
View file
Open in desktop
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) {
Copy link

Choose a reason for hiding this comment

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

Can this have a more descriptive name? This is replacing each term with an existing term, right?

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 its name may be simple here, since it is declared in the class.

"This replaces each term (or possibly recognizable expression) with an existing expression."

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.

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