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

[DRAFT][compiler] Introduce EliminateDeadSubgraphPass #13628

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

BalyshevArtem
Copy link
Contributor

This pr introduces EliminateDeadSubgraphPass for removing dead subgraph.

for issues: #12263 and for #13365
from draft: #13602

ONE-DCO-1.0-Signed-off-by: Artem Balyshev [email protected]

This pr introduces EliminateDeadSubgraphPass for removing dead subgraph.

ONE-DCO-1.0-Signed-off-by: Artem Balyshev <[email protected]>
@BalyshevArtem BalyshevArtem force-pushed the eliminate_dead_graph_pass_draft branch from 6d834f3 to 765fad1 Compare August 8, 2024 10:32
@@ -515,7 +515,7 @@ int entry(int argc, char **argv)
luci::change_outputs(graph, new_outputs);
}

// call luci optimizations for module
// call luci optimizations for module before optimizations for graph
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this comment changed to "before..." ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to emphasize that after graph optimization there will be another optimization of the entire module. If it only makes it more confusing, I can remove this comment :)

Copy link
Contributor

Choose a reason for hiding this comment

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

comment can give misunderstanding that one "MUST..."
if not "MUST", please restore as before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 59 to 60
auto *while_node = dynamic_cast<luci::CircleWhile *>(circle_node);
assert(while_node != nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

we can use loco::must_cast for this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines 73 to 74
auto *if_node = dynamic_cast<luci::CircleIf *>(circle_node);
assert(if_node != nullptr);
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

assert(body_graph_index >= 0);
assert(cond_graph_index >= 0);
// Add indexes into queue
reachable_graphs_indexes_q.push_back(size_t(body_graph_index));
Copy link
Contributor

@seanshpark seanshpark Aug 8, 2024

Choose a reason for hiding this comment

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

Q) why use size_t(body_graph_index) ? this will always be 4
plz use static_cast<size_t>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@seanshpark
Copy link
Contributor

Q) why separate EliminateDeadSubgraphPass to another draft?

@BalyshevArtem
Copy link
Contributor Author

Q) why separate EliminateDeadSubgraphPass to another draft?

In order to separate these two new optimizations. The current draft is completely independent of the previous one)

@seanshpark
Copy link
Contributor

The current draft is completely independent of the previous one)

But the FuseGeluPass will only work correctly with this pass, or there will be needless subgraphs.
Is this OK with you?

@BalyshevArtem
Copy link
Contributor Author

But the FuseGeluPass will only work correctly with this pass, or there will be needless subgraphs.
Is this OK with you?

No, we need to get rid of dead graphs, as this greatly inflates the binary size of the model.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants