-
Notifications
You must be signed in to change notification settings - Fork 10
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
Port Graph Optimizations #52
base: master
Are you sure you want to change the base?
Conversation
4032199
to
64e3612
Compare
50958e6
to
ddbd80e
Compare
ddbd80e
to
4f3da1c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
// Physical + x | ||
{std::make_pair<ConnectionType, ConnectionType>(Physical, Normal), Physical}, | ||
{std::make_pair<ConnectionType, ConnectionType>(Physical, Delayed), Invalid}, // !!! | ||
{std::make_pair<ConnectionType, ConnectionType>(Physical, Enclaved), PhysicalEnclaved}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all these cases should be invalid for now (except for Phycical + Normal)
// PhysicalEnclaved + x | ||
{std::make_pair<ConnectionType, ConnectionType>(PhysicalEnclaved, Normal), PhysicalEnclaved}, | ||
{std::make_pair<ConnectionType, ConnectionType>(PhysicalEnclaved, Delayed), Invalid}, // !!! | ||
{std::make_pair<ConnectionType, ConnectionType>(PhysicalEnclaved, Enclaved), PhysicalEnclaved}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all these cases should be invalid for now (except for PhycicalEnclaved + Normal)
@@ -37,7 +37,7 @@ public: | |||
virtual ~ReactorElement() = default; | |||
|
|||
// not copyable, but movable | |||
ReactorElement(const ReactorElement&) = delete; | |||
ReactorElement(const ReactorElement&) = default; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copy constructor is deleted on purpose and we should try to avoid enabling it
@@ -48,6 +48,13 @@ set_target_properties(${LIB_TARGET} PROPERTIES | |||
VERSION ${PROJECT_VERSION} | |||
SOVERSION 1) | |||
|
|||
option(GRAPH_OPTIMIZATIONS "Graph optimizations" ON) | |||
if(GRAPH_OPTIMIZATIONS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be handled more conveniently in config.hh.in
@@ -63,8 +63,20 @@ void Environment::register_input_action(BaseAction* action) { | |||
} | |||
|
|||
void Environment::optimize() { | |||
// no optimizations | |||
optimized_graph_ = graph_; | |||
#if GRAPH_OPTIMIZATIONS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Easier:
constexpr bool enable_optimizations = (GRAPH_OPTIMIZATIONS == 1);
@@ -39,10 +39,10 @@ ReactorElement::ReactorElement(const std::string& name, ReactorElement::Type typ | |||
container->register_action(reinterpret_cast<BaseAction*>(this)); // NOLINT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
container->register_action(reinterpret_cast<BaseAction*>(this)); // NOLINT | |
container->register_action(static_cast<BaseAction*>(this)); // NOLINT |
break; | ||
case Type::Output: | ||
container->register_output(reinterpret_cast<BasePort*>(this)); // NOLINT | ||
container->register_output(static_cast<BasePort*>(this)); // NOLINT | ||
break; | ||
case Type::Reaction: | ||
container->register_reaction(reinterpret_cast<Reaction*>(this)); // NOLINT |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
container->register_reaction(reinterpret_cast<Reaction*>(this)); // NOLINT | |
container->register_reaction(static_cast<Reaction*>(this)); // NOLINT |
std::copy_if(keys.begin(), keys.end(), std::back_inserter(has_upstreams), | ||
[](auto element) { return element->connected_to_upstream_actions(); }); | ||
|
||
// generating all the possible destinations for all sources |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be really helpful to have a more lengthy comment explaining the high level idea of the algorithm.
|
||
for (auto edge : path) { | ||
auto property = std::get<1>(edge); | ||
// auto source_port = std::get<0>(edge); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
TODO: Mermaid Function fixen |
831323e
to
fd8fda0
Compare
About
This pull requests add functionality, so the port graph can be first optimized before instantiating.
Current Algorithm
Create a spanning tree for every port and then squash the edges together so ideally only a tree of depth 1 remains.
Challanges
Given a port-graph like this:
The current graph optimizer would create something like this:
If B just forwards data and
Dead Path Elimination
Paths: (A-->D, A-->B and B-->C) can be eliminated with the following reasoning: