Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incorrect restructuring of multi-head loop #46

Closed
sklam opened this issue Apr 18, 2023 · 3 comments · Fixed by #53
Closed

Incorrect restructuring of multi-head loop #46

sklam opened this issue Apr 18, 2023 · 3 comments · Fixed by #53

Comments

@sklam
Copy link
Member

sklam commented Apr 18, 2023

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
Copy link
Member Author

sklam commented Apr 18, 2023

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

@esc
Copy link
Member

esc commented Apr 24, 2023

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
Copy link
Member

esc commented Apr 24, 2023

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

esc added a commit to esc/numba-rvsdg that referenced this issue Apr 26, 2023
Fixes numba#46

The problem was that, in case of a multi-header loop with a single exit,
the control-variable wasn't being setup correctly.

A further problem is that, even though this was tested, only the overall
block structure of the SCFG was being compared. However the instructions
of the blocks themselves were not, so this went unnoticed.

The fix for the test is to add the instructions contained within blocks
to the YAML and then we can test this properly.
esc added a commit to esc/numba-rvsdg that referenced this issue Jun 22, 2023
Fixes numba#46

The problem was that, in case of a multi-header loop with a single exit,
the control-variable wasn't being setup correctly.

A further problem is that, even though this was tested, only the overall
block structure of the SCFG was being compared. However the instructions
of the blocks themselves were not, so this went unnoticed.

The fix for the test is to add the instructions contained within blocks
to the YAML and then we can test this properly.
@esc esc closed this as completed in #53 Jun 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants