fix: keep raised conditional branches in depth-first order - #4
Conversation
`ControlFlowRaising._lift_conditionals` collected the blocks of a branch with `branch_nodes = set(dfs_conditional(...))`. `dfs_conditional` yields a deterministic depth-first ordering, but control flow blocks do not override `__hash__`, so a plain `set` orders them by `id()` — which differs between processes. That order is not merely internal: it drives `branch.add_nodes_from(branch_nodes)` and, via `graph.all_edges(*branch_nodes)`, the order the branch's edges are re-added in. Two identical lowerings of the same program could therefore produce SDFGs that serialize differently and hash differently, and could pick a different one of the two outgoing edges to collapse into the unconditional `else` branch. Note that `all_edges` already builds its result in an `OrderedSet`. That is not enough on its own, because an ordered container only preserves the order it is given; the ordering has to be kept at the point where it is first discarded. Unlike the `set` ordering addressed in spcl#2445, this one does not depend on `PYTHONHASHSEED` — identity hashes vary with address space layout instead — so it survives a fixed hash seed. Found in gt4py, whose compile cache is keyed on the serialized SDFG: across five runs of the icon4py dycore and diffusion benchmarks, one program of 67 alternated between two forms, differing by which branch of a conditional carried the negated condition. Only this one `set` is changed. The others in the pass are used for membership tests or set arithmetic and do not reach the emitted SDFG.
Verified end-to-endRe-ran the icon4py dycore + diffusion benchmarks on GH200, 5 runs with distinct
For contrast, the same workload before these two fixes: One negative result worth recording, since it is the obvious first thing to try: running |
|
Superseded by spcl#2495, which carries this fix plus the unstructured-lifting path. |
What
ControlFlowRaising._lift_conditionalscollected the blocks of a branch withdfs_conditionalyields a deterministic depth-first ordering, but control flowblocks do not override
__hash__, so a plainsetorders them byid()— which differsbetween processes.
That order is not internal bookkeeping. It drives
branch.add_nodes_from(branch_nodes)and, through
graph.all_edges(*branch_nodes), the order the branch's edges are re-addedin. Two identical lowerings of the same program could therefore produce SDFGs that
serialize differently and hash differently, and could pick a different one of the two
outgoing edges to collapse into the unconditional
else.Why the existing
OrderedSetdid not already cover thisGraph.all_edgesalready accumulates into anOrderedSet(dace/sdfg/graph.py:219).That is not sufficient on its own: an ordered container only preserves the order it is
given, and here it is handed
*branch_nodes, whose order was already discarded. Theordering has to be kept at the point where it is first lost.
Worth noting because it is the natural place to look first, and it looks like the problem
is already solved there.
Difference from spcl#2445
That PR replaced
sets whose elements are strings, where iteration order is governed byPYTHONHASHSEED. This one is identity-hashed, so it varies with address space layoutand survives a fixed hash seed — it needs
setarch -R(or an ordered container) topin down, not
PYTHONHASHSEED.Evidence
Found in gt4py, whose compile cache is keyed on the serialized SDFG. Across five runs of
the icon4py dycore + diffusion benchmarks on GH200, one program of 67
(
compute_rayleigh_w) alternated between two forms in a 3-vs-2 split. The difference:i.e. the two branches of the conditional swapping which one carried the negated
condition, taking a ~311-line block with them. It reaches the generated CUDA, not just
the SDFG.
Tests
test_raised_branch_preserves_dfs_orderbuilds a conditional whose taken branch is achain of 8 blocks, raises it, and asserts the raised branch keeps the chain order. On
mainthe blocks come out scrambled (branch_block_6first); with the fix they are inorder. The chain is long enough that the old behaviour cannot plausibly match by chance.
Note the 7 pre-existing
CompilationErrorfailures in that file in my environment are alocal toolchain issue (
cmake -G Ninja, no ninja on PATH) and are identical with andwithout this change: 8 failed before, 7 failed after, the difference being this test.
Scope
Only the one
setthat reaches the emitted SDFG is changed. The others in this pass(
addedfor membership, and the sets in the unstructured-lifting path, whose output isbuilt from
cfg.edges()graph order) do not affect the serialized result and are leftalone.