fix[next-dace]: deterministic access node order in concat_where mapper - #2780
Conversation
`_setup_initial_producer_description_in_nested_state` collected the names of the data it still had to create access nodes for into a `set[str]`, then iterated that set to call `nested_state.add_access()`. That loop fixes the order the access nodes are inserted into the state, and therefore the order they appear in the serialized SDFG. `str` hashing is salted per process, so the same program lowered twice could produce SDFGs that differ and hash differently. The build cache is keyed on the serialized SDFG, so affected programs missed the cache on every fresh process. Observed on icon4py's `test_graupel_only` (R2B05, dace_gpu). Two independent runs produced four distinct fingerprints for the two compiled programs, differing by 16 lines in a ~200'000 line SDFG: two access nodes for `t_out` and `t_out_0` swapping position in the state feeding a `concat_where` tasklet, with identical debuginfo and identical wiring otherwise. Sort the names, matching what this module already does for the same reason where consumer scopes are ordered.
tehrengruber
left a comment
tehrengruber
left a comment
There was a problem hiding this comment.
I would propose the OrderedSet again. Sorting of indeterministic strings lead to performance indeterminism, whereas ordered sets don't care.
Sorting produces a deterministic order, but one unrelated to the dataflow, and these names embed generated numbering. Keeping the insertion order of the producer specs preserves the natural order instead, and does not trade non-deterministic codegen for non-deterministic performance.
|
Switched to Your reasoning is better than mine and I have dropped the argument I made in the
|
…k pass `_eliminate()` uses the `FindAccessNodes` result instead of scanning every state. The pass returns a `set` per state and its iteration order decides in which order the new AccessNodes are inserted, so the loop sorts by `state.node_id`, see GridTools#2779 and GridTools#2780. `_accesses_region()` uses the shared `maybe_intersecting()` and the removal of `T` is validated under the GT4Py debug flag rather than DaCe's. The class docstring described the requirement on `G` as a window between the definition of `T` and the write back. It has to hold from the definition onwards, which is also what serves a consumer that reads after the write back; that was enforced but left to be inferred. It is now stated as the simplification it is. The comment on the subset size was wrong: `Range.size()` does divide by the step. The check holds because the source subset equals `Range.from_array()` and so has unit steps, which the comment now says.
2 participants
The helper _setup_initial_producer_description_in_nested_state collected the data names it still had to create access nodes for into a set and then iterated it. That iteration decides the order the access nodes are inserted into the state so the same program could produce SDFGs differing in node order between processes.
The fix uses an OrderedSet, so the access nodes follow the order of the producer specs.