Bug in `RefineNestedAccess` and `SDFGState._read_and_write_sets()`

[philip-paul-mueller]

During the implementation of the new MapFusion transformation it became necessary to reimplement _read_and_write_sets() there several bugs were discovered and fixed. However, one bug could not be fixed that is related to how the read set is cleaned.

If the following patch is applied

From f61d4e142b2b2097250b77e5b56b636b67ac9219 Mon Sep 17 00:00:00 2001
From: Philip Mueller <philip.mueller@cscs.ch>
Date: Fri, 6 Sep 2024 15:23:59 +0200
Subject: [PATCH] Undo the compatibility mode in _read_and_write_sets

---
 dace/sdfg/state.py | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/dace/sdfg/state.py b/dace/sdfg/state.py
index 23ac2f763..0c912d4d1 100644
--- a/dace/sdfg/state.py
+++ b/dace/sdfg/state.py
@@ -791,16 +791,6 @@ class DataflowGraphView(BlockGraphView, abc.ABC):
                 # Filter out memlets which go out but the same data is written to the AccessNode by another memlet
                 for out_edge in list(out_edges):
                     for in_edge in in_edges:
-                        if out_edge.data.data != in_edge.data.data:
-                            # NOTE: This check does not make any sense, and is in my view wrong.
-                            #   If we consider a memlet between two access nodes, to which access
-                            #   node the `data` attribute of the memlet refers to is arbitrary and
-                            #   does not matter. However, the test will filter _some_ out but not
-                            #   all. See also the tests inside `tests/sdfg/state_test.py` for the
-                            #   wrong behaviour this check induces.
-                            #   This check is is retained for  compatibility with `RefineNestedAccess`,
-                            #   see `tests/numpy/ufunc_support_test.py::test_ufunc_add_accumulate_simple`.
-                            continue
                         if in_subsets[in_edge].covers(out_subsets[out_edge]):
                             out_edges.remove(out_edge)
                             break
-- 
2.46.0

then tests/transformations/move_loop_into_map_test.py::MoveLoopIntoMapTest::test_apply_multiple_times will fail with an invalid SDFG error. The bug is probably located somewhere inside RefineNestedAccess.

Another error that surfaces is in tests/numpy/ufunc_support_test.py::test_ufunc_add_accumulate_simple, however, it only surfaces if the config variables optimizer.automatic_simplification and optimizer.autooptimize are set to True.

Since the regression test (see below) respect the bug it will also fail, in addition tests/transformations/move_loop_into_map_test.py::MoveLoopIntoMapTest::test_more_than_a_map will also fail, because now a transformation, that was previously classified as not applicable, became applicable (actually here I am not sure, but since my regression tests show that the old implementation is not correct, I am pretty positive).

• Commit introduced the backward compatibility
• Commit introducing regression tests

[philip-paul-mueller]

After I merged master into the new-map-fusion branch some tests that were previously failing (if the bug described here is fixed) no longer failed, thus in commit 5c49eee66 I removed the check, the tests should now still pass.

It turned out, it does not run, there was an error in the tests/numpy/ufunc_support_test.py::test_ufunc_add_accumulate_simple test, I forgot about it as it only happens in autoopt mode.

[philip-paul-mueller]

See also commit e1f2bf6 for an example where this behaviour is kicking in.

[acalotoiu]

Prune connectors is one of the transformations that has repeatedly caused problems and I believe is not currently used because of it. Maybe we should deprecate or fix @phschaad ?

[philip-paul-mueller]

Do you mean RefineNestedAccesse or PruneConnectors. My comment about the commit was that now PC was behaving the old way, i.e. should an input connector be pruned or not. Furthermore as far as I know PC is not run by default and GT4Py depends on it to make inline nested SDFG working.

[phschaad]

Prune connectors is one of the transformations that has repeatedly caused problems and I believe is not currently used because of it. Maybe we should deprecate or fix @phschaad ?

If PC is indeed the issue, I would vote for fixing since I understand that it still serves a purpose, but to be honest I am not very familiar with either PC or RNA..

[philip-paul-mueller]

@lukastruemper Has made some fixes to the transformation, he can probably give more insights.