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

Map written LocationSets to program locations (loc_t) instead of IR::Expression*s #4797

Merged
merged 5 commits into from
Jul 19, 2024
Merged

Map written LocationSets to program locations (loc_t) instead of IR::Expression*s #4797

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b9ef6d9
Map written LocationSets to program locations (loc_t) instead of IR::…
kfcripps Jul 12, 2024
655b9c2
Use hvec_map instead of ordered_map for writes and use std::unordered…
kfcripps Jul 12, 2024
867382a
Short-circuit when parents are equal
kfcripps Jul 13, 2024
8e55157
code cleanup
kfcripps Jul 17, 2024
6858f16
Memoize loc_t hashes
kfcripps Jul 17, 2024
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
53 changes: 47 additions & 6 deletions frontends/p4/def_use.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -638,8 +638,10 @@ bool ComputeWriteSet::preorder(const IR::SelectExpression *expression) {
visit(expression->select);
visit(&expression->selectCases);
auto l = getWrites(expression->select);
for (auto c : expression->selectCases) {
auto s = getWrites(c->keyset);
const loc_t *selectCasesLoc = getLoc(&expression->selectCases, getChildContext());
for (auto *c : expression->selectCases) {
const loc_t *selectCaseLoc = getLoc(c, selectCasesLoc);
auto s = getWrites(c->keyset, selectCaseLoc);
l = l->join(s);
}
expressionWrites(expression, l);
Expand Down Expand Up @@ -673,7 +675,8 @@ bool ComputeWriteSet::preorder(const IR::MethodCallExpression *expression) {
lhs = save;
auto mi = MethodInstance::resolve(expression, storageMap->refMap, storageMap->typeMap);
if (auto bim = mi->to<BuiltInMethod>()) {
auto base = getWrites(bim->appliedTo);
const loc_t *methodLoc = getLoc(expression->method, getChildContext());
auto base = getWrites(bim->appliedTo, methodLoc);
cstring name = bim->name.name;
if (name == IR::Type_Header::setInvalid || name == IR::Type_Header::setValid) {
// modifies only the valid field.
Expand Down Expand Up @@ -712,7 +715,7 @@ bool ComputeWriteSet::preorder(const IR::MethodCallExpression *expression) {
LOG3("Analyzing callees of " << expression << DBPrint::Brief << callees << DBPrint::Reset
<< indent);
ProgramPoint pt(callingContext, expression);
ComputeWriteSet cw(this, pt, currentDefinitions);
ComputeWriteSet cw(this, pt, currentDefinitions, cached_locs);
cw.setCalledBy(this);
for (auto c : callees) (void)c->getNode()->apply(cw);
currentDefinitions = cw.currentDefinitions;
Expand All @@ -735,7 +738,8 @@ bool ComputeWriteSet::preorder(const IR::MethodCallExpression *expression) {
visit(arg);
lhs = save;
if (p->direction == IR::Direction::Out || p->direction == IR::Direction::InOut) {
auto val = getWrites(arg->expression);
const loc_t *argLoc = getLoc(arg, getChildContext());
auto val = getWrites(arg->expression, argLoc);
result = result->join(val);
}
}
Expand All @@ -759,6 +763,43 @@ void ComputeWriteSet::visitVirtualMethods(const IR::IndexedVector<IR::Declaratio
}
}

std::size_t P4::loc_t::hash() const {
if (!computedHash) {
if (!parent)
computedHash = Util::Hash{}(node->id);
else
computedHash = Util::Hash{}(node->id, parent->hash());
}
return computedHash;
}

// Returns program location of n, given the program location of n's direct parent.
// Use to get loc if n is indirect child (e.g. grandchild) of currently being visited node.
// In this case parentLoc is the loc of n's direct parent.
const P4::loc_t *ComputeWriteSet::getLoc(const IR::Node *n, const loc_t *parentLoc) {
loc_t tmp{n, parentLoc};
return &*cached_locs.insert(tmp).first;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return &*cached_locs.insert(tmp).first;
return &*cached_locs.emplace(n, parentLoc).first;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was basically copied from the midend's version of loc_t. Maybe tmp was left for debugging purposes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, this doesn't seem to work:

/local/kfcripps/repos/p4c/frontends/p4/def_use.cpp:776:46:   required from here
/usr/include/c++/9/ext/new_allocator.h:146:4: error: new initializer expression list treated as compound expression [-fpermissive]
  146 |  { ::new((void *)__p) _Up(std::forward<_Args>(__args)...); }
      |    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/c++/9/ext/new_allocator.h:146:4: error: no matching function for call to 'P4::loc_t::loc_t(const P4::loc_t*&)'
In file included from /local/kfcripps/repos/p4c/frontends/p4/def_use.cpp:17:
/local/kfcripps/repos/p4c/frontends/p4/def_use.h:39:8: note: candidate: 'P4::loc_t::loc_t()'
   39 | struct loc_t {
      |        ^~~~~
/local/kfcripps/repos/p4c/frontends/p4/def_use.h:39:8: note:   candidate expects 0 arguments, 1 provided
/local/kfcripps/repos/p4c/frontends/p4/def_use.h:39:8: note: candidate: 'constexpr P4::loc_t::loc_t(const P4::loc_t&)'
/local/kfcripps/repos/p4c/frontends/p4/def_use.h:39:8: note:   no known conversion for argument 1 from 'const P4::loc_t*' to 'const P4::loc_t&'
/local/kfcripps/repos/p4c/frontends/p4/def_use.h:39:8: note: candidate: 'constexpr P4::loc_t::loc_t(P4::loc_t&&)'
/local/kfcripps/repos/p4c/frontends/p4/def_use.h:39:8: note:   no known conversion for argument 1 from 'const P4::loc_t*' to 'P4::loc_t&&'

Copy link
Contributor

Choose a reason for hiding this comment

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

well, you'd likely need to implement proper constructor ;)

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 was unsuccessful yet again: kfcripps@2275e49

Let me know if any suggestions on how to achieve this

}

// Returns program location given the context of the currently being visited node.
// Use to get loc of currently being visited node.
const P4::loc_t *ComputeWriteSet::getLoc(const Visitor::Context *ctxt) {
if (!ctxt) return nullptr;
loc_t tmp{ctxt->node, getLoc(ctxt->parent)};
return &*cached_locs.insert(tmp).first;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return &*cached_locs.insert(tmp).first;
return &*cached_locs.emplace(ctxt->node, getLoc(ctxt->parent)).first;

}

// Returns program location of a child node n, given the context of the
// currently being visited node.
// Use to get loc if n is direct child of currently being visited node.
const P4::loc_t *ComputeWriteSet::getLoc(const IR::Node *n, const Visitor::Context *ctxt) {
for (auto *p = ctxt; p; p = p->parent)
if (p->node == n) return getLoc(p);
auto rv = getLoc(ctxt);
loc_t tmp{n, rv};
return &*cached_locs.insert(tmp).first;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return &*cached_locs.insert(tmp).first;
return &*cached_locs.emplace(n, getLoc(ctxt)).first;

}

// Symbolic execution of the parser
bool ComputeWriteSet::preorder(const IR::P4Parser *parser) {
LOG3("CWS Visiting " << dbp(parser));
Expand All @@ -784,7 +825,7 @@ bool ComputeWriteSet::preorder(const IR::P4Parser *parser) {
// but we use the same data structures
ProgramPoint pt(state);
currentDefinitions = allDefinitions->getDefinitions(pt);
ComputeWriteSet cws(this, pt, currentDefinitions);
ComputeWriteSet cws(this, pt, currentDefinitions, cached_locs);
cws.setCalledBy(this);
(void)state->apply(cws);

Expand Down
183 changes: 114 additions & 69 deletions frontends/p4/def_use.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,26 @@ limitations under the License.

namespace P4 {

class ComputeWriteSet;
class StorageFactory;
class LocationSet;

// A location in the program. Includes the context from the visitor, which needs to
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Three slashes. Same goes the other comments.

// be copied out of the Visitor::Context objects, as they are allocated on the stack and
// will become invalid as the IR traversal continues
struct loc_t {
const IR::Node *node;
const loc_t *parent;
mutable std::size_t computedHash = 0;
bool operator==(const loc_t &a) const {
if (node != a.node) return false;
if (parent == a.parent) return true;
if (!parent || !a.parent) return false;
return *parent == *a.parent;
}
std::size_t hash() const;
};

/// Abstraction for something that is has a left value (variable, parameter)
class StorageLocation : public IHasDbPrint, public ICastable {
static unsigned crtid;
Expand Down Expand Up @@ -331,10 +348,14 @@ class ProgramPoint : public IHasDbPrint {
namespace std {
template <>
struct hash<P4::ProgramPoint> {
typedef P4::ProgramPoint argument_type;
typedef std::size_t result_type;
result_type operator()(argument_type const &s) const { return s.hash(); }
std::size_t operator()(const P4::ProgramPoint &s) const { return s.hash(); }
};

template <>
struct hash<P4::loc_t> {
std::size_t operator()(const P4::loc_t &loc) const { return loc.hash(); }
};

} // namespace std

namespace Util {
Expand Down Expand Up @@ -476,6 +497,65 @@ class AllDefinitions : public IHasDbPrint {
*/

class ComputeWriteSet : public Inspector, public IHasDbPrint {
public:
explicit ComputeWriteSet(AllDefinitions *allDefinitions)
: allDefinitions(allDefinitions),
currentDefinitions(nullptr),
returnedDefinitions(nullptr),
exitDefinitions(new Definitions()),
storageMap(allDefinitions->storageMap),
lhs(false),
virtualMethod(false),
cached_locs(*new std::unordered_set<loc_t>) {
CHECK_NULL(allDefinitions);
visitDagOnce = false;
}

// expressions
bool preorder(const IR::Literal *expression) override;
bool preorder(const IR::Slice *expression) override;
bool preorder(const IR::TypeNameExpression *expression) override;
bool preorder(const IR::PathExpression *expression) override;
bool preorder(const IR::Member *expression) override;
bool preorder(const IR::ArrayIndex *expression) override;
bool preorder(const IR::Operation_Binary *expression) override;
bool preorder(const IR::Mux *expression) override;
bool preorder(const IR::SelectExpression *expression) override;
bool preorder(const IR::ListExpression *expression) override;
bool preorder(const IR::Operation_Unary *expression) override;
bool preorder(const IR::MethodCallExpression *expression) override;
bool preorder(const IR::DefaultExpression *expression) override;
bool preorder(const IR::Expression *expression) override;
bool preorder(const IR::InvalidHeader *expression) override;
bool preorder(const IR::InvalidHeaderUnion *expression) override;
bool preorder(const IR::P4ListExpression *expression) override;
bool preorder(const IR::HeaderStackExpression *expression) override;
bool preorder(const IR::StructExpression *expression) override;
// statements
bool preorder(const IR::P4Parser *parser) override;
bool preorder(const IR::P4Control *control) override;
bool preorder(const IR::P4Action *action) override;
bool preorder(const IR::P4Table *table) override;
bool preorder(const IR::Function *function) override;
bool preorder(const IR::AssignmentStatement *statement) override;
bool preorder(const IR::ReturnStatement *statement) override;
bool preorder(const IR::ExitStatement *statement) override;
bool preorder(const IR::BreakStatement *statement) override;
bool handleJump(const char *tok, Definitions *&defs);
bool preorder(const IR::ContinueStatement *statement) override;
bool preorder(const IR::IfStatement *statement) override;
bool preorder(const IR::ForStatement *statement) override;
bool preorder(const IR::ForInStatement *statement) override;
bool preorder(const IR::BlockStatement *statement) override;
bool preorder(const IR::SwitchStatement *statement) override;
bool preorder(const IR::EmptyStatement *statement) override;
bool preorder(const IR::MethodCallStatement *statement) override;

const LocationSet *writtenLocations(const IR::Expression *expression) {
expression->apply(*this);
return getWrites(expression);
}

protected:
AllDefinitions *allDefinitions; /// Result computed by this pass.
Definitions *currentDefinitions; /// Before statement currently processed.
Expand All @@ -487,16 +567,17 @@ class ComputeWriteSet : public Inspector, public IHasDbPrint {
const StorageMap *storageMap;
/// if true we are processing an expression on the lhs of an assignment
bool lhs;
/// For each expression the location set it writes
hvec_map<const IR::Expression *, const LocationSet *> writes;
/// For each program location the location set it writes
hvec_map<loc_t, const LocationSet *> writes;
bool virtualMethod; /// True if we are analyzing a virtual method
AllocTrace memuse;
alloc_trace_cb_t nested_trace;
static int nest_count;

/// Creates new visitor, but with same underlying data structures.
/// Needed to visit some program fragments repeatedly.
ComputeWriteSet(const ComputeWriteSet *source, ProgramPoint context, Definitions *definitions)
ComputeWriteSet(const ComputeWriteSet *source, ProgramPoint context, Definitions *definitions,
std::unordered_set<loc_t> &cached_locs)
: allDefinitions(source->allDefinitions),
currentDefinitions(definitions),
returnedDefinitions(nullptr),
Expand All @@ -506,10 +587,14 @@ class ComputeWriteSet : public Inspector, public IHasDbPrint {
callingContext(context),
storageMap(source->storageMap),
lhs(false),
virtualMethod(false) {
virtualMethod(false),
cached_locs(cached_locs) {
visitDagOnce = false;
}
void visitVirtualMethods(const IR::IndexedVector<IR::Declaration> &locals);
const loc_t *getLoc(const IR::Node *n, const loc_t *parentLoc);
const loc_t *getLoc(const Visitor::Context *ctxt);
const loc_t *getLoc(const IR::Node *n, const Visitor::Context *ctxt);
void enterScope(const IR::ParameterList *parameters,
const IR::IndexedVector<IR::Declaration> *locals, ProgramPoint startPoint,
bool clear = true);
Expand All @@ -518,25 +603,39 @@ class ComputeWriteSet : public Inspector, public IHasDbPrint {
Definitions *getDefinitionsAfter(const IR::ParserState *state);
bool setDefinitions(Definitions *defs, const IR::Node *who = nullptr, bool overwrite = false);
ProgramPoint getProgramPoint(const IR::Node *node = nullptr) const;
const LocationSet *getWrites(const IR::Expression *expression) const {
auto result = ::get(writes, expression);
// Get writes of a node that is a direct child of the currently being visited node.
const LocationSet *getWrites(const IR::Expression *expression) {
const loc_t &exprLoc = *getLoc(expression, getChildContext());
auto result = ::get(writes, exprLoc);
BUG_CHECK(result != nullptr, "No location set known for %1%", expression);
return result;
}
// Get writes of a node that is not a direct child of the currently being visited node.
// In this case, parentLoc is the loc of expression's direct parent node.
const LocationSet *getWrites(const IR::Expression *expression, const loc_t *parentLoc) {
const loc_t &exprLoc = *getLoc(expression, parentLoc);
auto result = ::get(writes, exprLoc);
BUG_CHECK(result != nullptr, "No location set known for %1%", expression);
return result;
}
// Register writes of expression, which is expected to be the currently visited node.
void expressionWrites(const IR::Expression *expression, const LocationSet *loc) {
CHECK_NULL(expression);
CHECK_NULL(loc);
LOG3(expression << dbp(expression) << " writes " << loc);
if (auto it = writes.find(expression); it != writes.end()) {
const Context *ctx = getChildContext();
BUG_CHECK(ctx->node == expression, "Expected ctx->node == expression.");
const loc_t &exprLoc = *getLoc(ctx);
if (auto it = writes.find(exprLoc); it != writes.end()) {
BUG_CHECK(*it->second == *loc || expression->is<IR::Literal>(),
"Expression %1% write set already set", expression);
} else {
writes.emplace(expression, loc);
writes.emplace(exprLoc, loc);
}
}
void dbprint(std::ostream &out) const override {
if (writes.empty()) out << "No writes";
for (auto &it : writes) out << it.first << " writes " << it.second << Log::endl;
for (auto &it : writes) out << it.first.node << " writes " << it.second << Log::endl;
}
profile_t init_apply(const IR::Node *root) override {
auto rv = Inspector::init_apply(root);
Expand All @@ -555,63 +654,9 @@ class ComputeWriteSet : public Inspector, public IHasDbPrint {
}
}

public:
explicit ComputeWriteSet(AllDefinitions *allDefinitions)
: allDefinitions(allDefinitions),
currentDefinitions(nullptr),
returnedDefinitions(nullptr),
exitDefinitions(new Definitions()),
storageMap(allDefinitions->storageMap),
lhs(false),
virtualMethod(false) {
CHECK_NULL(allDefinitions);
visitDagOnce = false;
}

// expressions
bool preorder(const IR::Literal *expression) override;
bool preorder(const IR::Slice *expression) override;
bool preorder(const IR::TypeNameExpression *expression) override;
bool preorder(const IR::PathExpression *expression) override;
bool preorder(const IR::Member *expression) override;
bool preorder(const IR::ArrayIndex *expression) override;
bool preorder(const IR::Operation_Binary *expression) override;
bool preorder(const IR::Mux *expression) override;
bool preorder(const IR::SelectExpression *expression) override;
bool preorder(const IR::ListExpression *expression) override;
bool preorder(const IR::Operation_Unary *expression) override;
bool preorder(const IR::MethodCallExpression *expression) override;
bool preorder(const IR::DefaultExpression *expression) override;
bool preorder(const IR::Expression *expression) override;
bool preorder(const IR::InvalidHeader *expression) override;
bool preorder(const IR::InvalidHeaderUnion *expression) override;
bool preorder(const IR::P4ListExpression *expression) override;
bool preorder(const IR::HeaderStackExpression *expression) override;
bool preorder(const IR::StructExpression *expression) override;
// statements
bool preorder(const IR::P4Parser *parser) override;
bool preorder(const IR::P4Control *control) override;
bool preorder(const IR::P4Action *action) override;
bool preorder(const IR::P4Table *table) override;
bool preorder(const IR::Function *function) override;
bool preorder(const IR::AssignmentStatement *statement) override;
bool preorder(const IR::ReturnStatement *statement) override;
bool preorder(const IR::ExitStatement *statement) override;
bool preorder(const IR::BreakStatement *statement) override;
bool handleJump(const char *tok, Definitions *&defs);
bool preorder(const IR::ContinueStatement *statement) override;
bool preorder(const IR::IfStatement *statement) override;
bool preorder(const IR::ForStatement *statement) override;
bool preorder(const IR::ForInStatement *statement) override;
bool preorder(const IR::BlockStatement *statement) override;
bool preorder(const IR::SwitchStatement *statement) override;
bool preorder(const IR::EmptyStatement *statement) override;
bool preorder(const IR::MethodCallStatement *statement) override;

const LocationSet *writtenLocations(const IR::Expression *expression) {
expression->apply(*this);
return getWrites(expression);
}
private:
// TODO: Make absl::node_hash_set instead?
std::unordered_set<loc_t> &cached_locs;
Copy link
Contributor

Choose a reason for hiding this comment

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

or use an hvec_set

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 think there is any hvec_set in the tree

Copy link
Contributor

Choose a reason for hiding this comment

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

You need to ensure stable addresses of values around insertions. So, flat_hash_set is not an option here. You might want to give node_hash_set a try:

Suggested change
std::unordered_set<loc_t> &cached_locs;
absl::node_hash_set<loc_t, Util::Hash> &cached_locs;

Copy link
Contributor Author

@kfcripps kfcripps Jul 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We are not doing any iteration of cached_locs, so stable insertion value ordering does not matter here. If you think absl::node_hash_set would be better (e.g. for performance reasons), feel free to push your changes to this branch as I ran into some errors when trying your suggestion and I'd rather not spend the time debugging this unless there is a good reason to do so.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not about stable iteration order. The issue is much more subtle, but is very important: you are doing (though I would prefer ->first here):

    return &*cached_locs.insert(tmp).first;

Note that you are returning the address of hash map entry. Therefore you need to ensure this address will not change during insertions / deletions. STL unordered containers guarantee this (as each bucket is essentially a list). flat_hash_set does not guarantee this, node_hash_set does. See https://abseil.io/docs/cpp/guides/container for more information

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that you are returning the address of hash map entry. Therefore you need to ensure this address will not change during insertions / deletions.

Ok, I see what you mean now.

I'm not convinced that using std::unordered_set is a problem in the first place, so I will not spend the time trying to get node_hash_set to work, but feel free to push such changes to this branch if you know how to do it and think that it will improve the performance.

(but I will at least update the comment if you don't want to make these changes)

};

} // namespace P4
Expand Down
Loading