-
Notifications
You must be signed in to change notification settings - Fork 90
Clear initializers in constant folding pass #2668
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| 0.5.5 | ||
| 0.5.6 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1256,10 +1256,20 @@ def replace_node( | |
|
|
||
| # Record the names of the values that has contributed to the replacement | ||
| _record_contributing_values(node, replacement) | ||
|
|
||
| # Obtain the list of non-None inputs to the node before it is cleared by | ||
| # replace_nodes_and_values to check for unused initializers later. | ||
| node_inputs = [v for v in node.inputs if v is not None] | ||
|
|
||
| ir.convenience.replace_nodes_and_values( | ||
| root, node, [node], replacement.new_nodes, node.outputs, replacement.new_outputs | ||
| ) | ||
|
|
||
| if isinstance(root, ir.Graph): | ||
| # The old node should now be detached from the graph | ||
| assert node.graph is None | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need this assertion? This might be true in the current implementation ... but just wondering if we might ever want to replace [oldnode] by [newnode1, oldnode, newnode2], for example ... not sure of replace_nodes_and_values supports such a scenario ... but the logic here seems to be robust even in that case. But the assert would fail in that case. Anyway ... just a minor suggestion. Also: in principle, this logic could go into
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Good point - I agree. But also replace_nodes_and_values is more general. So checking value users may be more expensive there.
Not sure? replace_nodes_and_values will disconnect the old nodes (which breaks all the value connections) so I don't think it will work? |
||
| _clear_unused_initializers(node_inputs) | ||
|
|
||
| self._modified = True | ||
|
|
||
| # TODO: what about new opset_imports? | ||
|
|
@@ -1336,6 +1346,19 @@ def _sym_value_can_replace_graph_output( | |
| return True | ||
|
|
||
|
|
||
| def _clear_unused_initializers(values: Sequence[ir.Value]) -> None: | ||
| # Detach all inputs to the node, then check for unused initializers | ||
| for value in values: | ||
| if value is None or not value.is_initializer(): | ||
| continue | ||
|
|
||
| if not value.uses(): | ||
| assert value.is_initializer() | ||
| assert value.graph is not None | ||
| assert value.name is not None | ||
justinchuby marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| value.graph.initializers.pop(value.name) | ||
|
|
||
|
|
||
| @dataclasses.dataclass | ||
| class FoldConstantsResult(ir.passes.PassResult): | ||
| symbolic_value_map: dict[ir.Value, SymbolicValue] | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.