search

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

fixup case of backedge from header to header #52

Closed esc closed 1 year ago

esc commented 1 year ago

Fixes #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 commented 1 year ago

Could you please rebase this onto the current main. Thanks.

done

esc commented 1 year ago

It looks like the test is failing post-rebase, I'll have to debug this.

esc commented 1 year ago

OK, so here is the failing graph comparison, it seems like the graph is partially mirrored at the basic_block_1 -- the order of the jump targets seems reversed.

Screen Shot 2023-05-08 at 18 24 53
esc commented 1 year ago

It looks like with the merge of the remove_labels PR, the test harness has become stricter and exposed that this test had been working by accident. 08b019eaac56f0c0597af69f38603c680a6897d2 fixes -- basically on input we have [1, 2] and the link to 1 is replaced with a link to 5 -- so the expected jump_targets should be [5, 2] -- the commit fixes that!

kc611 commented 1 year ago

The changes LGTM! I think we can go ahead with the merge on this one.

esc commented 1 year ago

@kc611 thank you for the review!