Support common subexpression elimination pass (CSE)

[titaiwangms]

Fix #2105

For the logic, this PR follows https://github.com/pytorch/pytorch/blob/main/torch/fx/passes/dialect/common/cse_pass.py.

Essentially, this PR traverses the original graph and examines whether the values or the nodes are duplicated. If it's not, the value or the node is saved in mappings, and added to the new graph. If it is duplicated, the value or the node is replaced with the mapped/saved value or node. (FunctionalPass)

CSE subgraph is not supported: #2345.

[Copilot]

Pull Request Overview

This PR adds a common subexpression elimination (CSE) pass for ONNX graphs, updates the model serialization API to use metadata_props, and includes tests for the new pass.

• Rename meta_data_props parameter to metadata_props in model constructors and deserialization.
• Implement CommonSubexpressionEliminationPass in common_subexpression_elimination.py.
• Add unit tests for the CSE pass in common_subexpression_elimination_test.py.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
onnxscript/ir/serde.py	Renamed meta_data_props to metadata_props
onnxscript/ir/_core.py	Updated constructor parameter name to metadata_props
onnxscript/ir/passes/common/common_subexpression_elimination.py	Added CSE functional pass implementation
onnxscript/ir/passes/common/common_subexpression_elimination_test.py	Added tests covering CSE behavior

Comments suppressed due to low confidence (1)

onnxscript/ir/passes/common/common_subexpression_elimination.py:45

• [nitpick] Variable name input shadows the built-in input() function. Consider renaming to graph_input or similar for clarity.

        for input in old_graph.inputs:

[codecov]

Codecov Report

Attention: Patch coverage is 76.63043% with 43 lines in your changes missing coverage. Please review.

Project coverage is 70.16%. Comparing base (143c531) to head (9fd8948).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...es/common/common_subexpression_elimination_test.py	69.40%	41 Missing ⚠️
.../passes/common/common_subexpression_elimination.py	95.91%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2304      +/-   ##
==========================================
+ Coverage   70.11%   70.16%   +0.04%     
==========================================
  Files         196      198       +2     
  Lines       24662    24846     +184     
  Branches     2656     2670      +14     
==========================================
+ Hits        17292    17433     +141     
- Misses       6453     6495      +42     
- Partials      917      918       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[xadupre]

I don't know what the best is, but: what about changing inplace the node input (if two results are the same, then the second one can be replaced inplace with the first one). And then add a pass to remove the unused nodes.

[titaiwangms]

Changed to inplace pass. Subgraph is not yet tackled.

[justinchuby]

I think it is ok to leave subgraphs untouched.

[justinchuby]

Also: once the PR is done feel free to create one in onnx/ir-py to include the same logic. We can release 0.1 then.

It would be better to move the PR to https://github.com/onnx/ir-py at this point, after all tests passing

[titaiwangms]

Also: once the PR is done feel free to create one in onnx/ir-py to include the same logic. We can release 0.1 then.

It would be better to move the PR to https://github.com/onnx/ir-py at this point, after all tests passing

Optimizer seems still using the current repo passes.

[justinchuby]

Also: once the PR is done feel free to create one in onnx/ir-py to include the same logic. We can release 0.1 then.

It would be better to move the PR to onnx/ir-py at this point, after all tests passing

Optimizer seems still using the current repo passes.

Hopefully fixed by #2324, but let me know if not

[github-advanced-security]

CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

[titaiwangms]

Now I have to land the PR in onnx-ir first.

[gramalingam]

We can't rename it if is a graph input also. We could change this condition to new_value.is_graph_output() or new_value.is_graph_input() ... I think it won't show up in CSE, but will be necessary if we move this as an IR utility used in other scenarios.

[titaiwangms]

Is there a kind of case that we would cse graph inputs? In this PR, it's deleting nodes and affecting their outputs.

[titaiwangms]

I see what you mean

[titaiwangms]

I will add that in onnx/ir-py#36

[titaiwangms]

@gramalingam I don't see how new_value here could be graph_input. It has to be from a node output. Does that sound right?