Feat: Add API for Defining Symbolic Codegen Optimization Rules #812
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Benchmark Results (Julia vlts)Time benchmarks
Memory benchmarks
|
AayushSabharwal
requested changes
Nov 17, 2025
Member
AayushSabharwal
left a comment
AayushSabharwal
left a comment
There was a problem hiding this comment.
Would it also help if I changed CSE so that the i'th element can be referred to with something like Code.cse_var[i]?
| OptimizationRule(name, detector, transformer, priority) | ||
| Defines an optimization rule with: | ||
| - `name`: A string identifier for the optimization. |
Member
There was a problem hiding this comment.
If we're enforcing that the name is a string, should the field just be typed as ::String?
Member
There was a problem hiding this comment.
The same applies for priority. Why not just make it Int? Less prone to errors.
| The detector function should implement the signature | ||
| ```julia | ||
| detector(expr::Code.Let, state::Code.CSEState) -> Union{Nothing, Vector{<:AbstractMatched}} |
Member
There was a problem hiding this comment.
Why does this need to return a Union? Can we just always return a Vector and check isempty? We should also document exactly what expr and state are, what modifications we can/cannot make to them, etc.
Contributor
Author
There was a problem hiding this comment.
Nothing is easier to ID imo, and can be used to dispatch as a shortcut if needed.
| priority::P | ||
| end | ||
|
|
||
| abstract type AbstractMatched end |
Member
There was a problem hiding this comment.
Is there some sort of interface on AbstractMatched?
Contributor
Author
There was a problem hiding this comment.
It's more intended to be a part of the interface for OptimizationRule which returns matched objects that get passed to the transformation function.
Member
There was a problem hiding this comment.
Right, but if AbstractMatched has no contract it doesn't really have a reason to exist? Or will a contract be added in the future?
Contributor
Author
There was a problem hiding this comment.
The basic case is that the matched objects might contain wildly different fields/ data. It is specific to the optimization. What kind of contract did you have in mind? Its a container that passes information from the detection to the transformation.
src/code.jl
Outdated
|
|
||
| abstract type AbstractMatched end | ||
|
|
||
| function bank(dic, key, value) |
Member
There was a problem hiding this comment.
All of these utility functions should be documented. It's not clear what the arguments are or what the function is intended to do.
| Base.isempty(l::Code.Let) = isempty(l.pairs) | ||
|
|
||
| # Apply optimization rules during CSE | ||
| function apply_optimization_rules(expr, state::Code.CSEState, rules) |
Member
There was a problem hiding this comment.
The argument is named rules (along with the function name) but the function body treats it as a single rule?
Contributor
Author
There was a problem hiding this comment.
Ah yes, I previously had it as a for loop over the rules sorted by priority. That way we could pass a number of rules transforming the code at every step.I had run into the case that that would require the detection functions to handle things like aliasing, which they should be aware of anyway but does add some complexity at the get go. If preferred we can add that aa a requirement and I'll work on checking what code we have that could potentially run afowl of these subtleties
Contributor
Benchmark Results (Julia v1)Time benchmarks
Memory benchmarks
|
AayushSabharwal
merged commit 844e350
into
JuliaSymbolics:master
15 of 18 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds an API that can define and apply optimizing rules on existing IR. Also allows for downstream packages to use as a hook to add additional rules as needed.