fix[next-dace]: do not apply DoubleWriteRemover if the destination is read in the Map - #2816
Open
edopao wants to merge 2 commits into
Open
fix[next-dace]: do not apply DoubleWriteRemover if the destination is read in the Map#2816edopao wants to merge 2 commits into
DoubleWriteRemover if the destination is read in the Map#2816edopao wants to merge 2 commits into
Conversation
…is read in the Map The `DoubleWriteRemover` transformation distributes the final write of an intermediate transient buffer into the producing Map, i.e. the destination access node is written directly by each Map iteration. If the destination is also read inside the Map scope, this creates a write-after-read hazard: the read may observe the newly written value instead of the original one, because DaCe does not order independent branches within a Map body (this was observed to produce silently wrong results with larger fused maps, where the write was emitted before the read). Reject the transformation when any final consumer is read inside the Map scope. The scope scan recurses into nested scopes and skips ExitNodes, whose in-edges carry the data written by the scope rather than reads into it. Add a regression test that reproduces the issue: a Map producing a transient `b` in one branch and reading `c` in an independent branch, followed by the write back `c[:] = b[:]`. Without the fix the transformation applies, which the test now forbids.
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
1 participant
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.
Description
DoubleWriteRemoverdistributes the final write of an intermediate transient buffer into the producing Map, i.e. the destination access node is written directly by each Map iteration. If the destination is also read inside the Map scope, this 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 original 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): stencils computing an auxiliary output from a field they also update in place read the already-overwritten value.The fix rejects the transformation when any final consumer is read inside the Map scope. The scope scan recurses into nested scopes and skips
ExitNodes, whose in-edges carry the data written by the scope rather than reads into it.A unit test reproducing the hazard pattern is added: a Map produces a transient
bin one branch and readscthrough an independent branch, followed by the write backc[:] = b[:]. Without this fix the transformation is applied despite the hazard (the test fails withnb_applied == 1), with the fix the buffer is kept and the transformation does not apply.Requirements