Repository navigation
Conversation
d4c4686 to
3b0658d
Compare
3b0658d to
788525e
Compare
|
cscs-ci run default |
1 similar comment
|
cscs-ci run default |
1b31304 to
d72f949
Compare
|
cscs-ci run default |
1 similar comment
|
cscs-ci run default |
|
The failure in the RoCM CI ( |
|
cscs-ci run default |
2 similar comments
|
cscs-ci run default |
|
cscs-ci run default |
|
cscs-ci run dace-determinism |
|
cscs-ci run default |
|
cscs-ci run dace-determinism |
|
cscs-ci run default |
philip-paul-mueller
left a comment
philip-paul-mueller
left a comment
There was a problem hiding this comment.
It generally looks okay, but there are some things that must be changed.
|
cscs-ci run default |
edopao
left a comment
edopao
left a comment
There was a problem hiding this comment.
Thank you for the review comments. I have included a change in cartesian, as requested by Roman.
|
|
||
| # Symbols that the nested SDFG assigns itself, e.g. on an interstate edge or as a | ||
| # loop variable. They are not necessarily listed in `nsdfg.symbols`. | ||
| reassigned_symbols: set[str] = set() |
There was a problem hiding this comment.
I agree, what about inner_written_symbols?
| # ones in the parent SDFG (see `NestedSDFG.validate()`), so the symbols they | ||
| # use, e.g. in the shape, must have the same name and meaning inside. If the | ||
| # nested SDFG uses the name of the symbol for something else, either a data | ||
| # container or a symbol, including one it assigns itself, it is renamed there. |
There was a problem hiding this comment.
I had to push back the code agent several times to make it realize that renaming of the inner symbols is not unavoidable, and that the old way is also a viable solution. Finally, it got it, but still it prefers to rename the inner symbols, just in one pass:
Fair point — you're right that this is not forced by DaCe. NestedSDFG.validate() only requires the inner descriptor to be equivalent to the parent's after applying the symbol mapping, so keeping the old scheme (fresh symbol inside, mapped to the outer one, descriptor written in the fresh name) also passes validation. I verified this against 2.0.0b1 with a connector whose descriptor uses an inner-written symbol.
The switch to the 1:1 policy is a design tradeoff, and the comment now says exactly that: with 1:1 names, descriptors, offsets and subsets cross the scope boundary verbatim at every nesting level, and no rename-translation machinery is needed anywhere; with fresh incoming names we never scan the graph, but every object crossing the boundary must be rewritten through a per-level rename map — the machinery that was here before, which multiplies with nesting depth and was easy to apply inconsistently.
Your concern about rename robustness across an arbitrary SDFG is what made me hesitate too, but for the pinned DaCe the risk is now bounded and tested: renames are gathered into a single replace_dict call (fixes the repeated full scans), and I verified + regression-tested that replace_dict renames map parameters together with all uses (memlet subsets, tasklet code, ranges) — writing the test also exposed a real gap: map parameters were not detected as inner-bound names at all, which is now fixed.
ae9a3cb to
b11af05
Compare
|
cscs-ci run default |
b11af05 to
ae9a3cb
Compare
|
cscs-ci run default |
|
cscs-ci run default |
philip-paul-mueller
left a comment
philip-paul-mueller
left a comment
There was a problem hiding this comment.
It looks better but I still have some comments.
Also the concat where remapper is still strange, although I am still willing to accept it.
| # ones in the parent SDFG (see `NestedSDFG.validate()`), so the symbols they | ||
| # use, e.g. in the shape, must have the same name and meaning inside. If the | ||
| # nested SDFG uses the name of the symbol for something else, either a data | ||
| # container or a symbol, including one it assigns itself, it is renamed there. |
There was a problem hiding this comment.
You can tell your agent that his renaming will also introduce this "dicontinualities" as the renaming will stop at nested SDFGs, thus a symbol_mapping that was before {"name_that_will_be_renamed": "name_that_will_be_renamed"} will become {"name_that_will_be_renamed": "renamed_name"}.
64eb3a1 to
88d5310
Compare
philip-paul-mueller
left a comment
philip-paul-mueller
left a comment
There was a problem hiding this comment.
Some more comments.
Update
daceto v2.0-beta1, which introduces the concept of no-view nested SDFG: the data descriptor for global arrays inside a nested SDFG have to be the same as the outer data descriptors.ICON-blueline performance comparison w.r.t. main branch:
