Skip to content

Conversation

@justinchuby
Copy link
Collaborator

@justinchuby justinchuby commented Dec 23, 2025

Use IR apis and common passes. Eliminate the use of getattr in the touched files.

Fixed a bug where CSE would cause the same value to duplicate as graph outputs, which is invalid in onnx.

Removal of legacy helpers and predicates:

  • Removed unused or redundant helper predicates and functions such as _is_elem, _count_consumers, _find_next_consumer_idx, _nodes, and _inputs from both the test imports and test implementations, favoring direct use of IR containers and methods.

Direct use of IR API:

  • Replaced custom value constructor V_ir with the standard ir.val throughout all tests, ensuring consistency with the IR API.
  • Simplified graph and function construction by using ir.Function and passing functions directly to ir.Model, removing complex logic for function attachment.

Test logic and assertion modernization:

  • Updated all node and input/output container accesses to use direct iteration (list(g) or g.inputs, g.outputs) instead of helper functions, and used direct attribute access for names.
  • Updated assertions to reflect correct ONNX semantics, e.g., graph outputs must remain distinct objects, and adjusted tests accordingly.

Documentation update:

  • Updated the IR optimizer authoring guide to recommend using built-in IR methods and containers directly, and to avoid unnecessary helpers.

@enpasos

Signed-off-by: Justin Chu <[email protected]>
Signed-off-by: Justin Chu <[email protected]>
Signed-off-by: Justin Chu <[email protected]>
Signed-off-by: Justin Chu <[email protected]>
Signed-off-by: Justin Chu <[email protected]>
Signed-off-by: Justin Chu <[email protected]>
@justinchuby justinchuby requested a review from enpasos December 23, 2025 18:31
@justinchuby
Copy link
Collaborator Author

The only difference is this: The CSE pass in onnx-ir does not consider non deterministic ops. We may want to include them as an update.

@enpasos
Copy link
Owner

enpasos commented Dec 26, 2025

@justinchuby Thank you for the massive effort on this modernization — it’s absolutely the right direction for the codebase.

Unfortunately, I wasn’t able to get this branch to pass 100% of the regression tests in my environment. The remaining failures were primarily related to aggressive DCE pruning in nested conditionals, as well as some complex shape-inference edge cases (notably FFT / Scatter).

To unblock the migration, I’ve opened a derivative PR #152 (use-passes-stable) that cherry-picks your work but selectively reverts a few optimizations (DCE and global replacements) back to their legacy, safer behavior. The intent is to preserve correctness first while keeping the successful pass-based modernization intact.

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.

3 participants