[WIP] Added region aware testing

[kc611]

As titled, this PR adds region-aware testing by modifying the from_yaml and to_yaml methods of SCFG.

Following is the task list for this PR:

• [x] Refactor from_yaml and to_yaml to new format.
• [x] Make the methods block-type aware
• [x] Make the methods region aware
• [x] Adapt the test suite to the new methods
• [x] Add the capability to build graphs using the YAML format
• [x] Add a comparator for testing the graph accurately.
• [x] Test the logic by adding automated tests for fig3 and fig4

[esc]

• [ ] Add capability to build graphs using the YAML format

@kc611 I see there is code to go from YAML to SCFG -- is this what this bullet refers to?

[esc]

@kc611 also, The synthetic assignments need to save, which variable is being assigned. Also the synthetic branches and such need to store their table of variable -> destination. This is part of the graph and semantics so needs to be stores inside the YAML.

[kc611]

• [ ] Add capability to build graphs using the YAML format

@kc611 I see there is code to go from YAML to SCFG -- is this what this bullet refers to?

Yes, it refers to updating the from_yaml and from_dict methods for constructing SCFGs using a given YAML.

[esc]

• [ ] Add capability to build graphs using the YAML format

@kc611 I see there is code to go from YAML to SCFG -- is this what this bullet refers to?

Yes, it refers to updating the from_yaml and from_dict methods for constructing SCFGs using a given YAML.

OK, can you tick that checkbox, or is there still stuff missing?

[kc611]

It is still missing support for building region blocks.

I have updated the description to include the final goal of this PR i.e. to get an automated test for transformed SCFGs which in our case is fig3 and fig4.

[esc]

It is still missing support for building region blocks.

Ah, so you can't get a graph back from YAML that contains region blocks?

I have updated the description to include the final goal of this PR i.e. to get an automated test for transformed SCFGs which in our case is fig3 and fig4.

Yes, and test_scc.py too.

[esc]

@kc611 thank you for submitting this. I made two reviews with some sugggestions. Also, please don't forget https://github.com/numba/numba-rvsdg/pull/50#discussion_r1247687450

[esc]

Also, I am not sure that the values of the control variables are being tested. Using the following diff, I was expecting the tests to fail:

diff --git i/numba_rvsdg/tests/test_figures.py w/numba_rvsdg/tests/test_figures.py
index 82419f1d69..4e86d088d8 100644
--- i/numba_rvsdg/tests/test_figures.py
+++ w/numba_rvsdg/tests/test_figures.py
@@ -144,7 +144,7 @@ blocks:
         end: 22
     synth_asign_block_0:
         type: synth_asign
-        variable_assignment: {'control_var_0': 0}
+        variable_assignment: {'control_var_0': 1}
     synth_asign_block_1:
         type: synth_asign
         variable_assignment: {'control_var_0': 1}

But they do seem to pass?

[kc611]

Seems like testing for equality assertions for synthetic assignments and synthetic branches has exposed a flaky failure in SCFG loop restructuring. (This can be tested by running the figure 3 test multiple times until you get a failure.)

[esc]

Seems like testing for equality assertions for synthetic assignments and synthetic branches has exposed a flaky failure in SCFG loop restructuring. (This can be tested by running the figure 3 test multiple times until you get a failure.)

I think there are some fixes for this exact issue in: https://github.com/numba/numba-rvsdg/pull/57 -- I will investigate porting those fixes to this PR.

[esc]

Seems like testing for equality assertions for synthetic assignments and synthetic branches has exposed a flaky failure in SCFG loop restructuring. (This can be tested by running the figure 3 test multiple times until you get a failure.)

I think there are some fixes for this exact issue in: #57 -- I will investigate porting those fixes to this PR.

This patch "seems" to fix things. I've run it several times now and I was unable to reproduce the error:

diff --git i/numba_rvsdg/core/datastructures/scfg.py w/numba_rvsdg/core/datastructures/scfg.py
index 2d29025b45..d80b99ae07 100644
--- i/numba_rvsdg/core/datastructures/scfg.py
+++ w/numba_rvsdg/core/datastructures/scfg.py
@@ -617,7 +617,7 @@ class SCFG:
             # Need to create synthetic assignments for each arc from a
             # predecessors to a successor and insert it between the predecessor
             # and the newly created block
-            for s in set(jt).intersection(successors):
+            for s in sorted(set(jt).intersection(successors)):
                 synth_assign = self.name_gen.new_block_name(SYNTH_ASSIGN)
                 variable_assignment = {}
                 variable_assignment[branch_variable] = branch_variable_value

Looking again at the patch in #57 -- there are only three changes that are not related to rendering. The above is one of them. Perhaps this is sufficient to fix the instability of the current loop restructure implementation.

[esc]

Seems like testing for equality assertions for synthetic assignments and synthetic branches has exposed a flaky failure in SCFG loop restructuring. (This can be tested by running the figure 3 test multiple times until you get a failure.)

I think there are some fixes for this exact issue in: #57 -- I will investigate porting those fixes to this PR.

This patch "seems" to fix things. I've run it several times now and I was unable to reproduce the error:

diff --git i/numba_rvsdg/core/datastructures/scfg.py w/numba_rvsdg/core/datastructures/scfg.py
index 2d29025b45..d80b99ae07 100644
--- i/numba_rvsdg/core/datastructures/scfg.py
+++ w/numba_rvsdg/core/datastructures/scfg.py
@@ -617,7 +617,7 @@ class SCFG:
             # Need to create synthetic assignments for each arc from a
             # predecessors to a successor and insert it between the predecessor
             # and the newly created block
-            for s in set(jt).intersection(successors):
+            for s in sorted(set(jt).intersection(successors)):
                 synth_assign = self.name_gen.new_block_name(SYNTH_ASSIGN)
                 variable_assignment = {}
                 variable_assignment[branch_variable] = branch_variable_value

Looking again at the patch in #57 -- there are only three changes that are not related to rendering. The above is one of them. Perhaps this is sufficient to fix the instability of the current loop restructure implementation.

I pushed this change with 42434fd476dab58c94582c23a7a09635867bb1be

[esc]

@kc611 looks good, thank you, I fixed two minor typos in cadd031 and it's now ready to merge.