numba / numba-rvsdg

Numba compatible RVSDG (Regionalized Value State Dependence Graph) utilities.
https://numba-rvsdg.readthedocs.io/
BSD 2-Clause "Simplified" License
18 stars 7 forks source link

Incorrect restructuring of multi-head loop #46

Closed sklam closed 1 year ago

sklam commented 1 year ago

From DEBUGGRAPH=1 pytest numba_rvsdg/tests/test_mock_asm.py::test_mock_scfg_fuzzer_case9 in #42.

CFG:

Screenshot 2023-04-18 at 4 24 11 PM

Loop restructured:

Screenshot 2023-04-18 at 4 24 50 PM

The loop should switch between offset 4 and offset 6 blocks but there are no assignment to control-variable a within the loop.

sklam commented 1 year ago

Same problem in DEBUGGRAPH=1 pytest numba_rvsdg/tests/test_mock_asm.py::test_mock_scfg_fuzzer_case153 -s

esc commented 1 year ago

This graph variation is already present in the the test suite:

https://github.com/numba/numba-rvsdg/blob/main/numba_rvsdg/tests/test_transforms.py#L552

The input looks like:

Screen Shot 2023-04-24 at 13 58 42

And the output like:

Screen Shot 2023-04-24 at 13 59 01

We can see here, that the variable a isn't being set in the assignments. I initially though, that this would be the result of a double header in the loop, but looking at the Fig 3. tests we see that:

Screen Shot 2023-04-24 at 14 03 24

It does work correctly in this case. So my next guess is that this may be related to the loop having only a single exit.

The reason this was not caught by the testing suite, is because the YAML tests only compare the structure of the graph, but not the "contents" of each block. Additional work on the testing suite will be needed to allow for encoding this.

esc commented 1 year ago

I have created a patch to fix this issue, it was indeed the case, when there were two headers but only one exit:

diff --git i/numba_rvsdg/core/transformations.py w/numba_rvsdg/core/transformations.py
index b3d59811b6..0ba8b2a493 100644
--- i/numba_rvsdg/core/transformations.py
+++ w/numba_rvsdg/core/transformations.py
@@ -151,7 +151,7 @@ def loop_restructure_helper(scfg: SCFG, loop: Set[Label]):
                     # the correct blocks
                     variable_assignment = {}
                     variable_assignment[backedge_variable] = reverse_lookup(backedge_value_table, loop_head)
-                    if needs_synth_exit:
+                    if needs_synth_exit or headers_were_unified:
                         variable_assignment[exit_variable] = reverse_lookup(header_value_table, jt)
                     # Update the backedge block - remove any existing backedges
                     # that point to the headers, no need to add a backedge,

And the correct rendering looks like:

Screen Shot 2023-04-24 at 16 03 11

The blocks SyntheticAssignment10 and SyntheticAssignment12 now also set a value for variable a which is the variable to be used in the SyntheticHead6