Remove name conflicts in loop iterators#2326
Open
ThrudPrimrose wants to merge 22 commits into
Open
Conversation
tbennun
requested changes
May 12, 2026
| from typing import Union | ||
|
|
||
|
|
||
| def replace_symbol_by_name(expr: sp.Basic, old_name: str, new: Union[str, sp.Basic]) -> sp.Basic: |
Collaborator
There was a problem hiding this comment.
we have symbolic replace and subs etc.
Why the new function?
|
|
||
| @dace.properties.make_properties | ||
| @explicit_cf_compatible | ||
| class SSALoopIterators(ppl.Pass): |
| @explicit_cf_compatible | ||
| class SSALoopIterators(ppl.Pass): | ||
| loop_var_counter = 0 | ||
| FOR_IT_NAME = "_it" |
Collaborator
There was a problem hiding this comment.
should this be a class variable or just a global constant
Collaborator
Author
There was a problem hiding this comment.
I guess this one is a global constant, I updated accordingly.
| def should_reapply(self, modified: ppl.Modifies) -> bool: | ||
| return False | ||
|
|
||
| def _repl_recursive(self, cfg: ControlFlowRegion | dace.SDFG, loop_var: str, next_ssa_loop_var: str): |
Collaborator
There was a problem hiding this comment.
I feel like dace.sdfg.replace.replace_dict would do a good job here, if applied selectively
The rename in ``_apply_recursive`` is scoped to the LoopRegion via ``_rename_one_loop_var(cfg, ...)``, so after a successful pass the LoopRegion's body, init / condition / update statements, and every nested-SDFG ``symbol_mapping`` referencing the old name have been rewritten to ``_loop_it_<N>``. The default ``assign_loop_iterator_post_value=False`` path stops there. Problem: the frontend declares the original loop variable in the **enclosing** SDFG's symbol table (e.g. ``sdfg.symbols['i']``), one level above the LoopRegion. The scoped rename never touches that declaration. With the post-value epilogue disabled there is also no ``<old_name> = <post_value>`` interstate assignment to keep the symbol alive, so the declaration is dead -- yet it still leaks as a phantom free symbol on the enclosing NestedSDFG boundary, producing ``Missing symbols on nested SDFG: ['i']`` at validation time and blocking any pass that re-checks the SDFG (LoopToMap, MapFission, …). Fix: in the no-postamble branch only, after the rename, drop the declaration when it is provably dead -- ``old_name in sdfg.symbols`` AND ``old_name not in sdfg.free_symbols`` (the second clause is what prevents accidental removal if some reader survived). The default ``assign_loop_iterator_post_value=True`` path is unaffected: the epilogue's assignment keeps the symbol live, the ``not in sdfg.free_symbols`` check would refuse anyway, and -- to make the distinction structural rather than implicit -- the cleanup sits in the ``elif`` branch of the existing if/else on the property. Tests: - ``test_no_postamble_drops_dead_symbol_declaration``: with ``assign_loop_iterator_post_value=False`` the dead ``i`` declaration is gone from every body NestedSDFG and ``sdfg.validate()`` succeeds (without the fix it raises ``Missing symbols on nested SDFG: ['i']``). - ``test_postamble_preserves_symbol_declaration``: with the default ``True`` the postamble's assignment keeps ``i`` alive, the cleanup must NOT trigger; the declaration must survive.
The yakup/dev branch refactored test_postamble_preserves_symbol_declaration to use UniqueLoopIterators() directly (default post-value=True) instead of explicitly setting the attribute. Same semantics, 3 lines tighter. 10/10 unique_loop_iterators tests still pass.
…symbols`` The no-postamble cleanup branch in ``UniqueLoopIterators._apply_recursive`` removes ``old_name`` from ``sdfg.symbols`` once it is no longer referenced by any rename target. It gated on ``old_name not in sdfg.free_symbols``, but ``SDFG.free_symbols`` calls ``used_symbols(all_symbols=True)``, and ``ControlFlowRegion._used_symbols_internal`` unconditionally folds ``self.symbols.keys()`` back into the "free" set when ``all_symbols`` is True (the ``if all_symbols: free_syms |= set(self.symbols.keys())`` branch). The gate is therefore circular: the declared symbol always appears "free" by virtue of being declared, the cleanup never fires, and the dead declaration lingers until the validator catches it with ``Missing symbols on nested SDFG: ['<old_name>']`` on the enclosing NestedSDFG. Switch the gate to ``used_symbols(all_symbols=False)``, which excludes the ``sdfg.symbols.keys()`` self-fold and reflects only actual code-generation usage. With this gate the cleanup correctly removes ``old_name`` exactly when no memlet subset, tasklet body, interstate-edge condition/assignment or descriptor expression still references it. Reproducer: a per-row reduction with an inner accumulator (``for i: s = ...; for j: s += a[i, j-1] + a[i, j] + a[i, j+1]; b[i] = s``). Before this fix, the inner ``j`` lingered in the body NestedSDFG's ``sdfg.symbols`` post-rename and the SDFG failed validation. After the fix the declaration is removed, validation passes, and the numerical result matches the pure-numpy oracle.
…; fixes nested-SDFG symbol-mapping crash) The pass re-renamed every LoopRegion iterator on each invocation, including loops already in ``_loop_it_<N>`` form from an earlier run (the canonicalize pipeline runs UniqueLoopIterators twice: the ``clean`` and ``ssa`` stages). Re-renaming an already-unique iterator that a deeply-nested SDFG imports drops the import from the grandchild NSDFG's ``symbol_mapping`` (the re-key does not survive a second rename of an already-imported iterator), leaving a phantom free symbol -> "Missing symbols on nested SDFG: ['_loop_it_<N>']" at validation. Fix: skip renaming a loop variable already in ``_loop_it_*`` form. This makes the pass idempotent (a second application is a no-op) and avoids the destructive double-rename. New test test_idempotent_skips_already_unique_iterators covers it; the existing suite stays green.
…sambiguation Two improvements to the pass: * Replace the class-static ``_loop_var_counter`` (mutable global state that made iterator names depend on call/test order) with a per-call counter seeded just past any ``_loop_it_<N>`` already present anywhere in the SDFG tree (``_first_free_id``). Renames never collide with an existing unique name and the numbering is deterministic for a given SDFG regardless of prior runs. A fresh SDFG still yields ``_loop_it_0, _loop_it_1, ...``. * Re-disambiguate DUPLICATE ``_loop_it_<N>`` names: the idempotency skip-guard now skips only iterators that are unique, so two LoopRegions that share a name (e.g. left behind by a cloning pass) are renamed apart instead of aliasing (which would block downstream passes such as LoopToMap). Tests: drop the now-pointless ``_loop_var_counter`` resets, switch ``__main__`` to ``pytest.main``, and add four cases -- triply-nested counted loops (deep cascade + value preservation), a negative-step counted DO, a scan-seed collision (existing ``_loop_it_5`` beside ``i`` -> ``i`` becomes ``_loop_it_6``), and duplicate-iterator disambiguation (two LoopRegions sharing ``_loop_it_0`` -> distinct names).
…ue as a constructor argument UniqueLoopIterators had no __init__, so the only way to set the post-value knob was a post-construction attribute assignment. Add an __init__ taking assign_loop_iterator_post_value (default True, matching the Property) and switch the unit-test call sites to the constructor keyword.
… too ``_first_free_id`` scanned only ``LoopRegion`` loop variables when picking the next ``_loop_it_<N>`` id, ignoring ``MapEntry`` parameters. ``LoopToMap`` keeps a loop's ``_loop_it_<N>`` name as the map parameter, so when ``UniqueLoopIterators`` runs after a ``LoopToMap`` stage a map carries that name; reusing the id for a freshly-renamed loop aliased two distinct iteration variables and silently corrupted results (e.g. canonicalize's early-LoopToMap path on velocity_advection). Scan map parameters as well so the seeded counter avoids both. Adds two unit tests: a structural seed-past-map-parameter test and an end-to-end loop+map coexistence value-preservation test. (cherry picked from commit cf64e5d5e3b43843f33e655a0b0ff74c2de9fd70)
ThrudPrimrose
force-pushed
the
ssa_loop_iterators
branch
from
c6afb1a to
aa4aab0
Compare
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
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.
This pass ensures that there are no name conflicts among loop-range iterators in the SDFG.