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

Single block loops not placed into loop region #45

Closed
sklam opened this issue Apr 18, 2023 · 4 comments · Fixed by #52
Closed

Single block loops not placed into loop region #45

sklam opened this issue Apr 18, 2023 · 4 comments · Fixed by #52

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_case36 of #42 .

Single block loops are not restructured:

CFG:
Screenshot 2023-04-18 at 2 12 39 PM
After loop restructuring
Screenshot 2023-04-18 at 2 13 31 PM

@esc
Copy link
Member

esc commented Apr 20, 2023

This happens when there is a backedge from header to itself.

This is the reproducer:

        original = """
        "0":
            jt: ["1"]
        "1":
            jt: ["1", "2"]
        "2":
            jt: ["1", "3"]
        "3":
            jt: []
        """

And this is the rendering of it:

Screen Shot 2023-04-20 at 14 46 39

This, on the other hand is the rendering of the failed transform, as you can see the backedge from header to header has simply not been processed.

Screen Shot 2023-04-20 at 15 17 47

@esc
Copy link
Member

esc commented Apr 20, 2023

The following patch will fix this case:

diff --git i/numba_rvsdg/core/transformations.py w/numba_rvsdg/core/transformations.py
index b3d59811b6..e991de2ba2 100644
--- i/numba_rvsdg/core/transformations.py
+++ w/numba_rvsdg/core/transformations.py
@@ -143,7 +143,7 @@ def loop_restructure_helper(scfg: SCFG, loop: Set[Label]):
                     # matters in this case.
                     new_jt[new_jt.index(jt)] = synth_assign
                 # If the target is the loop_head
-                elif jt in headers and label not in doms[jt]:
+                elif jt in headers: #and label not in doms[jt]:
                     # Create the assignment and record it
                     synth_assign = SynthenticAssignment(str(scfg.clg.new_index()))
                     new_blocks.add(synth_assign)

And yield the following, correct, result:

Screen Shot 2023-04-20 at 15 26 45

However, this breaks the fig.3 test with:

    def replace_jump_targets(self, jump_targets: Tuple) -> "BasicBlock":
        fallthrough = len(jump_targets) == 1
        old_branch_value_table = self.branch_value_table
        new_branch_value_table = {}
        for target in self.jump_targets:
            if target not in jump_targets:
                # ASSUMPTION: only one jump_target is being updated
                diff = set(jump_targets).difference(self.jump_targets)
>               assert len(diff) == 1
E               AssertionError

numba_rvsdg/core/datastructures/basic_block.py:94: AssertionError

@esc
Copy link
Member

esc commented Apr 20, 2023

However, this breaks the fig.3 test with:

    def replace_jump_targets(self, jump_targets: Tuple) -> "BasicBlock":
        fallthrough = len(jump_targets) == 1
        old_branch_value_table = self.branch_value_table
        new_branch_value_table = {}
        for target in self.jump_targets:
            if target not in jump_targets:
                # ASSUMPTION: only one jump_target is being updated
                diff = set(jump_targets).difference(self.jump_targets)
>               assert len(diff) == 1
E               AssertionError

numba_rvsdg/core/datastructures/basic_block.py:94: AssertionError

This is the same error as in: #48

@esc
Copy link
Member

esc commented Apr 20, 2023

An alternative fix that doesn't break the test suite may be:

diff --git i/numba_rvsdg/core/transformations.py w/numba_rvsdg/core/transformations.py
index b3d59811b6..a630ef45cb 100644
--- i/numba_rvsdg/core/transformations.py
+++ w/numba_rvsdg/core/transformations.py
@@ -143,7 +143,7 @@ def loop_restructure_helper(scfg: SCFG, loop: Set[Label]):
                     # matters in this case.
                     new_jt[new_jt.index(jt)] = synth_assign
                 # If the target is the loop_head
-                elif jt in headers and label not in doms[jt]:
+                elif jt in headers and (label not in doms[jt] or label == jt):
                     # Create the assignment and record it
                     synth_assign = SynthenticAssignment(str(scfg.clg.new_index()))
                     new_blocks.add(synth_assign)

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

Headers of multi-block loops that have a backedge to themselves, didn't
have this processed.

The case of:

`label not in doms[jt]`

accounts for the case, where there were multiple entry edges into the
loop and the original header has been replaced with a synthetic one.

An alternative fix would have been to simply delete this check, but then
this breaks the test case `test_double_header` (after hacking the
followup issues). This fix is non-intrusive and a test for the edge
cases has been added too.
esc added a commit to esc/numba-rvsdg that referenced this issue May 4, 2023
Fixes numba#45

Headers of multi-block loops that have a backedge to themselves, didn't
have this processed.

The case of:

`name not in doms[jt]`

accounts for the case, where there were multiple entry edges into the
loop and the original header has been replaced with a synthetic one.

An alternative fix would have been to simply delete this check, but then
this breaks the test case `test_double_header` (after hacking the
followup issues). This fix is non-intrusive and a test for the edge
cases has been added too.
esc added a commit to esc/numba-rvsdg that referenced this issue May 4, 2023
Fixes numba#45

Headers of multi-block loops that have a backedge to themselves, didn't
have this processed.

The case of:

`name not in doms[jt]`

accounts for the case, where there were multiple entry edges into the
loop and the original header has been replaced with a synthetic one.

An alternative fix would have been to simply delete this check, but then
this breaks the test case `test_double_header` (after hacking the
followup issues). This fix is non-intrusive and a test for the edge
cases has been added too.
esc added a commit to esc/numba-rvsdg that referenced this issue May 4, 2023
Fixes numba#45

Headers of multi-block loops that have a backedge to themselves, didn't
have this processed.

The case of:

`name not in doms[jt]`

accounts for the case, where there were multiple entry edges into the
loop and the original header has been replaced with a synthetic one.

An alternative fix would have been to simply delete this check, but then
this breaks the test case `test_double_header` (after hacking the
followup issues). This fix is non-intrusive and a test for the edge
cases has been added too.
@esc esc closed this as completed in #52 May 29, 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