-
Notifications
You must be signed in to change notification settings - Fork 72
[pass] Update DCE passes #2257
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
[pass] Update DCE passes #2257
Conversation
❌ 8 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
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.
Pull Request Overview
This pull request refactors the dead code elimination passes to prevent unintended modifications to the model signature by removing the now-irrelevant remove_initialized_inputs option and ensuring the default opset is always retained.
- Removed the removal of initialized inputs and associated constructor parameters in RemoveUnusedNodesPass.
- Modified initializer removal logic to protect inputs in the model signature.
- Updated domain handling in _process_graph_like to always retain the default opset.
Co-authored-by: Copilot <[email protected]>
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.
Might need to update tests such as
onnxscript/onnxscript/ir/passes/common/unused_removal_test.py
Lines 105 to 107 in b63ba43
model = self.remove_unused_nodes( | |
model, remove_initialized_inputs=remove_initialized_inputs | |
) |
and/or the function called in these tests
onnxscript/onnxscript/optimizer/__init__.py
Lines 115 to 127 in b63ba43
def remove_unused_nodes( | |
model: ir.Model | onnx.ModelProto, remove_initialized_inputs: bool = False | |
) -> None: | |
"""Removes unused nodes from a model inplace.""" | |
if isinstance(model, ir.Model): | |
onnxscript.ir.passes.common.unused_removal.RemoveUnusedNodesPass( | |
remove_initialized_inputs=remove_initialized_inputs | |
)(model) | |
else: | |
model_ir = ir.serde.deserialize_model(model) | |
model_ir = onnxscript.ir.passes.common.unused_removal.RemoveUnusedNodesPass( | |
remove_initialized_inputs=remove_initialized_inputs | |
)(model_ir).model |
to reflect the removal of the attribute
@shubhambhokare1 thanks! Done. |
model = ir.serde.serialize_model(model_ir) | ||
return model | ||
onnxscript.optimizer.remove_unused_nodes(model, remove_initialized_inputs) | ||
onnxscript.optimizer.remove_unused_nodes(model) | ||
return model | ||
|
||
def test_remove_unused_nodes(self): |
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.
There should a test with a model including subgraphs.
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.
Thanks - will update
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.
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.
From the discussion with Kunal on phi4_mm/builder.py, It seems from the user side, they don't really benefit from delicately/cautiously split passes. The end of the day, they just call the higher level API. So that means these delicately/cautiously split passes only serve as debugging usage for ourselves. From debugging point of view, would it be too trivial to separate unused initializers from DCE?
I don't think the passes are designed for debugging purposes only. There can be users who want a high level api which we can provide, but we will also support usages that are more fine grained & users can design their own transformation pipelines. Each pass should be modularized: it should have a clear contract and a self contained behavior. |
remove_initialized_inputs
option in dce because the contract of the pass it that it does not modify model signature. Fixed bugs where initializers are removed. Instead, users can use Add add/remove initializers passes #2253 to remove the initialized inputs first.