Detect and reroll unrolled do-while loops #76

matt-kempster · 2020-04-26T04:53:32Z

Don't merge this yet - just putting some WIP work up.

matt-kempster · 2020-05-01T18:17:08Z

This is in a stable state. The scope doesn't need to get much bigger for this PR; fixing &&/|| detection and more complex do-while detection can be in different PRs.

matt-kempster · 2021-06-25T04:39:35Z

So far this diff comprises:

reroll.py, which needs to be refactored so that it doesn't suck
- I have some local code which can help this, namely regarding replacing graphs with subgraphs in an automated way
changes to translate.py that are ostensibly all bugs and need to be diagnosed and fixed
adding --no-reroll as a CLI arg
adding replace_any_children() to BaseNode: is this acceptable?
adding a test that doesn't really need to be there (I should remove it)

zbanks

This is great - I really like how it's relatively small/readable it feels for what it's doing.

src/flow_graph.py

zbanks · 2021-06-25T16:03:30Z

src/flow_graph.py

            for s in n.children():
                if s.loop is not None and n in s.loop.backedges:
                    continue
-                incoming_edges[s].remove(n)
+                try:
+                    incoming_edges[s].remove(n)
+                except KeyError:
+                    break


At first glance, I think the break should be continue (if it's OK to have missing edges) or return False (if it's not OK) -- otherwise, right now the behavior seems dependent on the order of the children in the list?

I think the fix here might actually be to iterate over set(children) instead. If n isn't in incoming_edges[s], then I think that means s.parents was not constructed correctly? (Sorry that this might be a pre-existing bug)

for s in set(n.children()): if s.loop is not None and n in s.loop.backedges: continue incoming_edges[s].remove(n)

Does that mean n.children() might have repeats? That makes sense actually, and is probably why my extremely old version of this PR didn't need this fix (I changed children() to a Set.)

Oh wait, I'm wrong, my bad!

n.children() shouldn't have repeats: the implementations for that fn in SwitchNode & ConditionalNode explicitly deduplicate the children.

I can take a look and figure out what's going on here.

src/translate.py

src/loop_rerolling.py

zbanks · 2021-06-25T16:11:56Z

src/loop_rerolling.py

+            if changed:
+                break
+    compute_relations(flow_graph.nodes)
+    return flow_graph


This function modifies flow_graph in-place, so I don't think it needs to return anything.

src/flow_graph.py

src/translate.py

tests/end_to_end/loop_nested/irix-o2-out.c

src/loop_rerolling.py

simonlindholm · 2021-06-25T20:06:44Z

src/loop_rerolling.py

+    node_2 = node_1.fallthrough_edge
+    node_5 = node_1.conditional_edge
+
+    if not isinstance(node_2, BasicNode):


I think we should also check that these nodes have the parents we expect. I feel like this pass has a large potential for misdetection and changing the asm into something non-equivalent, so we should be as careful as we can in the pattern-matching.

simonlindholm · 2021-06-25T20:08:40Z

src/loop_rerolling.py

+    replace_node_references(flow_graph, replace_this, with_this)
+
+
+def reroll_loop(flow_graph: FlowGraph, start: ConditionalNode) -> bool:


Add a comment with the structure/asm we're trying to pattern-match here (or refer to a test?).

It'd be nice to include a dot representation of what's going on:

digraph before { start -> node_1 [label=F, color=blue]; start -> node_7 [label=T, color=red]; node_1 -> node_2 [label=F, color=blue]; node_1 -> node_5 [label=T, color=red]; node_2 -> node_3; node_3 -> node_4 [label=F, color=blue]; node_3 -> node_3 [label=T, color=red]; node_4 -> node_5 [label=F, color=blue]; node_4 -> node_7 [label=T, color=red]; node_5 -> node_6; node_6 -> node_7 [label=F, color=blue]; node_6 -> node_6 [label=T, color=red]; } digraph after { start -> new_node_1 [label=F, color=blue]; start -> node_7 [label=T, color=red]; new_node_1 -> node_2 [label=F, color=blue]; new_node_1 -> node_7 [label=T, color=red]; node_2 -> node_3; node_3 -> node_7 [label=F, color=blue]; node_3 -> node_3 [label=T, color=red]; }

And maybe a link to an online render, like https://bit.ly/3A2MEJt

This is actually nearly exactly what the new reroll.py does -- stay tuned

zbanks · 2021-07-01T13:46:49Z

src/loop_rerolling.py

+    indices = [node.block.index for node in flow_graph.nodes]
+    assert sorted(indices) == indices, "FlowGraphs should be sorted"
+
+    offset = start.block.index


start may not be equal to flow_graph.nodes[start.block.index]: maybe instead use flow_graph.nodes.index(start) (and catch the error & return None)?

Right now this can happen when ReturnNodes are duplicated, but unrolling loops would also mess up the block indexing.

zbanks · 2021-07-01T13:49:54Z

src/loop_rerolling.py

+    return tuple(
+        node
+        for i, node in enumerate(flow_graph.nodes)
+        if node is not start and i in all_nodes_in_pattern


I don't think start should be excluded?

If you include start in the output, then when you call nodes = detect_pattern(...), nodes[i] refers to the node i in the pattern. (Without it, nodes[0] refers to node 1, etc.)

zbanks · 2021-07-01T13:52:21Z

src/loop_rerolling.py

+    return tuple(
+        node
+        for i, node in enumerate(flow_graph.nodes)
+        if node is not start and i in all_nodes_in_pattern
+    )


Suggested change

return tuple(

node

for i, node in enumerate(flow_graph.nodes)

if node is not start and i in all_nodes_in_pattern

)

return tuple(flow_graph.nodes[i] for i in sorted(all_nodes_in_pattern))

zbanks · 2021-07-01T13:53:58Z

src/loop_rerolling.py

+def remove_and_replace_nodes(flow_graph: FlowGraph, nodes: Tuple[Node, ...]) -> None:
+    (node_1, node_2, node_3, node_4, node_5, node_6, node_7) = nodes
+    new_node_1 = BasicNode(node_1.block, node_1.emit_goto, node_2)
+    replace_node(flow_graph, node_1, new_node_1)
+    remove_node(flow_graph, node_4, node_7)
+    remove_node(flow_graph, node_5, node_7)
+    remove_node(flow_graph, node_6, node_7)  # TODO: assert didn't execute anything?.


This function is only used once, and looks pretty specific to the exact structure: I think it'd be easier if the body were just inlined directly in reroll_loop?

zbanks · 2021-07-01T14:05:00Z

src/loop_rerolling.py

+    # TODO: Check that the subgraph is self-contained except for the entry
+    # and exit nodes, which themselves should be somehow "designated" using an enum.


I think the following check is sufficient, without requiring you to explicitly mark an exit node in the pattern? (I think an exit node is any node that is not in pattern.keys() but is in pattern.values()? And the start node is 0?)
I think it can be up to the caller to check start.parents: I don't think the start's parents matter in this case.

# pattern_nodes could be the `tuple(flow_graph.nodes[i] for i in sorted(all_nodes_in_pattern))` return value I suggest later for node in pattern_nodes: if node is not start and not all(p in pattern_nodes for p in node.parents): return None

matt-kempster added 7 commits April 25, 2020 20:16

Add --no-andor for loop.c, and a similar looping test case

0de7b12

Initial infrastructure for detecting for-loops

9d10258

Use do-while, not for; screwed up indentation

d394e9b

Fix screwed up indentation

ad167f7

Add complicated loop unrolling; break -g indentation

8d99b31

Actually run tests

24f5e2f

Fix indentation of simple do-whiles

e35518b

matt-kempster added do_not_merge WIP labels Apr 26, 2020

matt-kempster added 16 commits April 26, 2020 16:20

Negate loop-gating condition

db72018

Merge with master

9bf11da

Divorce if-statement from do-while loop

b3b8e22

Add missing semicolon after do-while

09e85cb

Delete a bunch of dead code

0a1156b

Introduce crazy loop-rerolling with an option to disable

03a8ab0

Replace --no-andor with --no-reroll in loop test

0b22145

Got loop.c working, but broke nested

c937919

Temp commit to see what used to work

b4ed8e0

Fix everything

75ce71f

CRLF -> LF

1997ed4

Remove the code from if_statements that was moved

7919a49

More CRLF

f637685

Another flip-flop, using --no-reroll for a test

2d58683

Refactor all spaghetti AND fix a bug

8a5bf38

Re-enable do-while emission

2fe2c7d

matt-kempster changed the title ~~[WIP] Detect for and while loops and avoid &&/|| crashes~~ Detect and reroll unrolled do-while loops May 1, 2020

matt-kempster removed WIP do_not_merge labels May 1, 2020

Temporary while commit

24e6066

zbanks mentioned this pull request Jun 16, 2021

Add single exit node (TerminalNode) to FlowGraph #114

Merged

matt-kempster added 7 commits June 23, 2021 20:16

Merge master into for_loops (using imerge)

03b6d42

Fix indentation of do-while loops

90c8f2c

Fix self_loop definition

4379021

Minor fixups

a2ed466

Fix bug where emitting a node kills that node forever

fb23b5d

Fix bug where nodes were not thought to be emitted

774d467

Fix mypy issue

914479e

matt-kempster mentioned this pull request Jun 24, 2021

Implement simple do-while loop functionality #117

Merged

matt-kempster added 4 commits June 24, 2021 20:55

Merge master into for_loops (using imerge)

ea1593f

Remove secretly=...

75a6c19

Remove secretly=... but correctly

5849d59

Remove secretly=... but correctly... again

4fdaadc

zbanks requested changes Jun 25, 2021

View reviewed changes

simonlindholm reviewed Jun 25, 2021

View reviewed changes

matt-kempster added 13 commits June 30, 2021 15:56

Remove old wrong comments

25e798f

Undo the translate.py copy import

2f9dcd0

Undo all translate.py 'fixes'

66083c3

Remove to_basic_node()

85e9d55

Use zbanks's suggestion in SwitchNode

b595ce8

Move compute_relations() to 'if changed' block

ec17645

Factor out the matching of nodes logic

ef59716

Implement drop-in replacement for match_nodes

fc7fff9

Delete match_nodes()

d63b4af

Delete idx_eq()

0df65fc

De Morgan's simplification

c89f8a0

Introduce remove_and_replace_nodes()

f9b25ed

Leave a note to myself

587e770

zbanks reviewed Jul 1, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect and reroll unrolled do-while loops #76

Detect and reroll unrolled do-while loops #76

matt-kempster commented Apr 26, 2020

matt-kempster commented May 1, 2020

matt-kempster commented Jun 25, 2021

zbanks left a comment

zbanks Jun 25, 2021

matt-kempster Jun 25, 2021

zbanks Jun 25, 2021

zbanks Jun 25, 2021

simonlindholm Jun 25, 2021

simonlindholm Jun 25, 2021

zbanks Jun 25, 2021

matt-kempster Jun 25, 2021

zbanks Jul 1, 2021

zbanks Jul 1, 2021

zbanks Jul 1, 2021

zbanks Jul 1, 2021

zbanks Jul 1, 2021

		replace_node_references(flow_graph, replace_this, with_this)


		def reroll_loop(flow_graph: FlowGraph, start: ConditionalNode) -> bool:

		# TODO: Check that the subgraph is self-contained except for the entry
		# and exit nodes, which themselves should be somehow "designated" using an enum.

Detect and reroll unrolled do-while loops #76

Are you sure you want to change the base?

Detect and reroll unrolled do-while loops #76

Conversation

matt-kempster commented Apr 26, 2020

matt-kempster commented May 1, 2020

matt-kempster commented Jun 25, 2021

zbanks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment