Cleanup new-IR based constant propagation #1739
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1739 +/- ##
==========================================
- Coverage 74.98% 74.95% -0.04%
==========================================
Files 245 245
Lines 26393 26384 -9
Branches 4804 4803 -1
==========================================
- Hits 19792 19777 -15
- Misses 5679 5684 +5
- Partials 922 923 +1 ☔ View full report in Codecov by Sentry. |
Test Results 24 files ± 0 24 suites ±0 3h 16m 37s ⏱️ + 10m 31s For more details on these failures, see this check. Results for commit 324300a. ± Comparison against base commit c37e98b. This pull request removes 3625 tests.♻️ This comment has been updated with latest results. |
|
|
||
|
|
||
| def replace_nodes_and_values( | ||
| graph_or_function: _core.Graph | _core.Function, |
There was a problem hiding this comment.
| graph_or_function: _core.Graph | _core.Function, | |
| graph_or_function: _core.Graph | _core.Function, | |
| /, |
There was a problem hiding this comment.
Done (but not pushed yet). But is there a specific principle being used here, to distinguish between the two classes of parameters? Except for the name, which is admittedly not great (we could do with a better alternative to graph_or_function ... how does container sound? or graph_like ?)
There was a problem hiding this comment.
I like graph_like!
is there a specific principle being used here, to distinguish between the two classes of parameters
not sure what you mean? could you say more?
There was a problem hiding this comment.
I mean, why do you prefer that graph_or_function be positional, while the others are allowed to be positional or by keyword?
There was a problem hiding this comment.
allowing graph_or_function by keyword can restrict our ability to change the name of the variable (which we have done: graph -> graph_or_function); but other options have more stable names.
| @@ -395,3 +395,44 @@ def create_value_mapping(graph: _core.Graph) -> dict[str, _core.Value]: | |||
| continue | |||
| values[value.name] = value | |||
| return values | |||
|
|
|||
|
|
|||
| def replace_nodes_and_values( | |||
There was a problem hiding this comment.
Helpful to have a test?
| old_values: list[_core.Value], | ||
| new_values: list[_core.Value], | ||
| ): | ||
| """Replaces nodes and values in the graph or function. |
There was a problem hiding this comment.
Helpful to describe the invariance this function maintains?
|
Is there a way to copy the metadata_props from old nodes as well, to preserve namespace and source information? |
Factor out some common logic between rewriter and constant-propagation into a utility function, and other minor cleanup.