Repository navigation
Conversation
… read GT4PyMapBufferElimination inlines the write-back 'MapExit -> tmp -> G' into the Map body. When the Map also reads 'G' through a branch that is independent of the branch producing the written value, the rewrite creates a write-after-read hazard: independent branches inside a Map scope can be scheduled in arbitrary (insertion) order, so the read may observe the newly written value and produce silently wrong results. Reject the rewrite unless every read of 'G' inside the Map is dataflow-ordered before the branch that produces the written value. Reads that feed the producing branch (in-place patterns such as 'G[i] = f(G[i])') remain admissible. Observed as wrong numerical results in icon4py stencil tests with the dace backend (e.g. 'extrapolate_temporally_exner_pressure'). The added unit test fails without this fix because the transformation is applied despite the hazard.
philip-paul-mueller
left a comment
philip-paul-mueller
left a comment
There was a problem hiding this comment.
I like the changes and think this is a good addition, but I think that the change is applied at the wrong place.
This issue is not new and to solve it gt_create_local_double_buffering() has been created.
Since the error surfaced the transformation did not kicked in.
Thus I would suggest to patch that one, although I am open to approve this PR as well.
|
|
||
| This function will also consider the `assume_pointwise` property. | ||
| """ | ||
| map_exit: dace_nodes.MapExit = self.map_exit |
There was a problem hiding this comment.
If we are at it: This access should only be performed once, i.e. do it in can_be_applied()/apply() and pass it as argument to everywhere where it is needed.
| """ | ||
| map_exit: dace_nodes.MapExit = self.map_exit | ||
| map_entry: dace_nodes.MapEntry = state.entry_node(map_exit) | ||
| glob_ac: dace_nodes.AccessNode = self.glob_ac |
There was a problem hiding this comment.
Same as for map_exit.
| through which `glob` is read. | ||
|
|
||
| """ | ||
| map_exit: dace_nodes.MapExit = self.map_exit |
There was a problem hiding this comment.
Either use state.exit_node() or even better add it as an argument.
|
|
||
| # The producing branch of the written value: the in-edges of the | ||
| # MapExit connector that corresponds to the matched out edge. | ||
| map_to_tmp_edge = next(iter(state.in_edges(self.tmp_ac))) |
There was a problem hiding this comment.
Same as for the other, accessing pattern nodes, i.e. writing self.tmp_ac, is slow and unstable.
Do it once in the beginning and then reuse it, by passing it around.
I know it is ugly, but it is faster and safer.
| inner_producer_edges = state.in_edges_by_connector(map_exit, "IN_" + written_conn[4:]) | ||
| producer_nodes = {edge.src for edge in inner_producer_edges} |
There was a problem hiding this comment.
| inner_producer_edges = state.in_edges_by_connector(map_exit, "IN_" + written_conn[4:]) | |
| producer_nodes = {edge.src for edge in inner_producer_edges} | |
| producer_nodes = {edge.src for edge in state.in_edges_by_connector(map_exit, "IN_" + written_conn[4:])} |
As far as I remember in_edges_by_connector() returns a one pass iterator.
Writing it this way ensures that it is not used multiple times.
|
|
||
| # Compute the producing branch and everything dataflow upstream of it. | ||
| # Reads in this set complete before the write is performed. | ||
| scope_nodes = set(state.all_nodes_between(map_entry, map_exit)) |
There was a problem hiding this comment.
You do not need scope_nodes instead bellow you simply check if in_edge.src is map_entry.
Good point, addressed in #2941 |
… double buffering (#2941) ## Problem `_check_if_map_must_be_handled` in `local_double_buffering.py` bailed on the **entire map** (`return None`) whenever a single inout data failed the subset compatibility check (`all_inner_subsets[0] == all_inner_subsets[i]`). This was inconsistent with the other two checks in the same loop (the "already buffered" check and the "not scalar" check), which correctly did `inout_datas.pop(inout_data_name); continue` to skip just the failing data. As a result, in a Map with multiple inout datas, if one data had mismatched read/write subsets, `return None` suppressed double buffering for **all** inout datas — including ones that genuinely need it to prevent a write-after-read (WAR) hazard after `GT4PyMapBufferElimination` inlines the write-back into the Map body. ## Fix Changed `return None` → `inout_datas.pop(inout_data_name); continue` in `_check_if_map_must_be_handled` (`local_double_buffering.py:418-419`), consistent with the two other checks in the same loop. This implements the reviewer's (philip-paul-mueller) preferred approach on PR #2815: > "I like the changes and think this is a good addition, but I think that the change is applied at the wrong place. This issue is not new and to solve it `gt_create_local_double_buffering()` has been created. Since the error surfaced the transformation did not kicked in. Thus I would suggest to patch that one." Instead of blocking the buffer elimination in `simplify.py` (PR #2815's approach), the fix lets `GT4PyMapBufferElimination(assume_pointwise=True)` proceed and relies on `gt_create_local_double_buffering` to insert a local double buffer that sequences the read before the write. ## Test Added `test_local_double_buffering_war_hazard` in `test_create_local_double_buffering.py`: - Builds a WAR hazard SDFG where a single Map reads `G` on one branch and writes `G` (via a `tmp` buffer) on an independent branch: `O(i) = G(i) + 1.0` and `G(i) = 2.0 * A(i)`. - Applies `GT4PyMapBufferElimination(assume_pointwise=True)` — asserts it fires (inlines the write-back). - Applies `gt_create_local_double_buffering` — asserts it fires (inserts the double buffer). - Executes the SDFG and verifies `O = G_old + 1.0` and `G = 2.0 * A` (the reader sees the OLD value of G, not the newly written one). ## QA - `test_create_local_double_buffering.py`: **6 passed** (5 existing + 1 new) - `test_map_buffer_elimination.py`: **10 passed** - Full `transformation_tests/` directory: **334 passed, 3 xfailed** - `pre-commit run` (ruff, mypy, tach, license): **clean** - `uv run mypy src/gt4py/next/program_processors/runners/dace/transformations/local_double_buffering.py`: **no issues** - `uv run tach check`: **all modules validated**
This branch has not been deployed
2 participants
Description
GT4PyMapBufferEliminationinlines the write-backMapExit -> tmp -> Ginto the Map body, eliminating the transient buffer. When the Map also readsGthrough a branch that is independent of the branch producing the written value, the rewrite creates a write-after-read hazard: independent branches inside a Map scope can be scheduled in arbitrary (insertion) order, so the read may observe the newly written value instead of the old one, producing silently wrong results.This showed up as wrong numerical output in the icon4py stencil tests with the dace backend (e.g.
extrapolate_temporally_exner_pressure): a stencil computing an auxiliary field from a field it also updates in place read the already-overwritten value.The fix adds a check to
_perform_pointwise_test: the rewrite is rejected unless every read ofGinside the Map is dataflow-ordered before the branch that produces the written value (new helper_map_reads_glob_before_write). Reads that feed the producing branch — in-place patterns of the formG[i] = f(G[i])produced by a single branch — are ordered before the write by construction and remain admissible, so existing valid uses are unaffected.A unit test reproducing the hazard pattern is added: without this fix the transformation is applied despite the hazard (test fails), and execution of the rewritten SDFG yields numerically wrong results.
Requirements