feat(ssa refactor): dominator tree #1253

joss-aztec · 2023-04-28T18:21:56Z

Related issue(s)

Resolves #1116

Description

Summary of changes

Add PostOrder for calculating the post-order of a function reachable blocks
Adds DominatorTree for querying one block's dominance over another
Make finished Ssa's function collection accessible so that the FunctionBuilder can be used in unit tests

Dependency additions / changes

Test additions / changes

Unit test for both of the above

Checklist

I have tested the changes locally.
I have formatted the changes with Prettier and/or cargo fmt with default settings.
I have linked this PR to the issue(s) that it resolves.
I have reviewed the changes on GitHub, line by line.
I have ensured all changes are covered in the description.

Documentation needs

This PR requires documentation updates when merged.

Additional context

kevaundray

Some preliminary comments

kevaundray · 2023-04-28T18:27:32Z

crates/noirc_evaluator/src/ssa_refactor/ir/dom/post_order.rs

+    Last,
+}
+
+// Calculates the post-order of the


Noting the unfinished sentence here (for when it comes out of draft)

kevaundray · 2023-04-28T18:28:50Z

crates/noirc_evaluator/src/ssa_refactor/ir/dom/post_order.rs

@@ -0,0 +1,136 @@
+use std::collections::HashSet;


can you put a paragraph at the top of this using //! ... to explain what post-order is and why it is needed?

kevaundray · 2023-04-28T18:30:00Z

crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs

@@ -0,0 +1,443 @@
+use std::{cmp::Ordering, collections::HashMap};


same here for dominator tree -- ie why it is needed and other useful information that would help someone understand the codebase. See at the top of some of Cranelifts files for inspiration on how this is done

jfecher

Looks pretty good so far. The tests looks good as well.

crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs

jfecher · 2023-04-28T20:25:29Z

crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs

+            DomNode { reverse_post_order_idx: 0, immediate_dominator: None },
+        );
+        for (i, &block_id) in entry_free_post_order.iter().rev().enumerate() {
+            // Indices have been displaced by 1 by to the removal of the entry node


Typo here, "by 1 by to the removal"

jfecher · 2023-04-28T20:34:33Z

crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs

+
+use crate::ssa_refactor::ir::{basic_block::BasicBlockId, function::Function};
+
+/// DFT stack state marker for computing the cfg post-order.


Can we spell out the acronym here?

jfecher · 2023-04-28T20:39:24Z

crates/noirc_evaluator/src/ssa_refactor/ir/post_order.rs

+                        // Stack successors for visiting. Because items are taken from the top of the
+                        // stack, we push the item that's due for a visit first to the top.
+                        for successor_id in
+                            func.dfg[block_id].successors().collect::<Vec<_>>().into_iter().rev()


successors should already be an iterator so we should be able to do:

func.dfg[block_id].successors().rev()

I had to add DoubleEndedIterator to successors return type.

jfecher · 2023-04-28T20:42:20Z

crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs

+        let cfg = ControlFlowGraph::with_function(&func);
+        let post_order = PostOrder::with_function(&func);
+        let dom_tree = DominatorTree::with_cfg_and_post_order(&cfg, &post_order);


Seems like this is a common operation. Are we expected to keep around the cfg and post_order blocks without the dominator tree? If not we can have a helper function to create the first two and pass them into the third instead of creating all three separately.

I thought I'd come across a use case of post order independent of dom trees - but possibly I imagined it. What cranelift does is to have the dom tree compute and take ownership of the post order. It then has an accessor for when the two are used in conjunction. Would that be preferred?

Either way I'll add a helper for the cfg.

Not my imagination - cranelift's dead code elimination pass iterates over post-order without any need for dom trees.

jfecher · 2023-04-28T20:42:58Z

crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs

+    immediate_dominator: Option<BasicBlockId>,
+}
+
+impl DomNode {


Since we're spelling out DominatorTree I think we should spell out DominatorNode or DominatorTreeNode as well.

crates/noirc_evaluator/src/ssa_refactor/ir/dom.rs

- Comment tweaks - rm unneeded collect iter into vec - clippy changes - DominatorTree::with_function helper - rename DomNode DominatorTreeNode

* Implement first-class functions * Update crates/noirc_evaluator/src/ssa_refactor/ir/dfg.rs Co-authored-by: kevaundray <[email protected]> * Implement intrinsics * Fix no return & duplicate main * bad git. remove duplicated functions * Remove Option<Function> in builder * Undo debug printing in driver * Fix loading from mutable parameters * Grammar * Fix storing to mutable arrays * Fix unused variable * Fix array loading * Change terminology --------- Co-authored-by: kevaundray <[email protected]>

* chore(ci): Utilize new workflow to build binaries * Update release.yml --------- Co-authored-by: Tom French <[email protected]>

* chore(noir): Release 0.5.0 * chore: Update lockfile

#1226) * chore: replace `aztec_backend` with `acvm-backend-barretenberg` * feat: update to ACVM 0.10.0 * chore: move `ComputeMerkleRoot` to same match arm as `HashToField128Security` * chore: bump backend commit * feat: update stdlib to use new merkle black box function * fix: bump commit of barretenberg to match acvm-backend-barretenberg * feat: update `merkle_insert` to use new `compute_merkle_root` function * chore: update to use ACVM 0.10.3 * chore: bump backend commit

add poseidon examples into nargo_cli

* chore(noir): Release 0.5.1 * chore: Update lockfile

… pass (#1256) * Add remaining doc comments * Update crates/noirc_evaluator/src/ssa_refactor/ir/cfg.rs Co-authored-by: kevaundray <[email protected]> * Update crates/noirc_evaluator/src/ssa_refactor/ir/cfg.rs Co-authored-by: kevaundray <[email protected]> * Address PR feedback --------- Co-authored-by: kevaundray <[email protected]>

* chore(noir): constrain expr; -> assert(expr); * chore(noir): replace remaining `constrain` with `assert(expr)` --------- Co-authored-by: Tom French <[email protected]>

TomAFrench · 2023-05-02T15:35:23Z

Whoops, I merged master into this branch and fixed the issue in f8a8933. My version of this branch turned out to be a little behind so I git pull --rebase'd and that seems to have split apart the merge commit.

joss-aztec · 2023-05-02T15:53:40Z

The file diff has exploded. I'm gonna close and cherry-pick into a new branch/PR.

joss-aztec · 2023-05-02T16:01:49Z

Replacement PR: #1278

joss-aztec added 2 commits April 28, 2023 19:21

feat(ssa refactor): dominator tree

0e19779

reorder functions

5cdc370

kevaundray reviewed Apr 28, 2023

View reviewed changes

joss-aztec added 6 commits April 28, 2023 19:38

chore(ssa refactor): use true post order

769cf0d

chore(ssa refactor): externalise PostOrder

70b5c7d

chore(ssa refactor): use FunctionBuilder in tests

c62c744

chore(ssa refactor): add header comments

8e74e48

chore(ssa refactor): clippy

407902b

chore(ssa refactor): elaborate header comments

92c8c10

joss-aztec marked this pull request as ready for review April 28, 2023 20:21

jfecher reviewed Apr 28, 2023

View reviewed changes

TomAFrench reviewed May 2, 2023

View reviewed changes

joss-aztec and others added 12 commits May 2, 2023 11:31

chore(ssa refactor): domtree/post-order cleanup:

12d2d6b

- Comment tweaks - rm unneeded collect iter into vec - clippy changes - DominatorTree::with_function helper - rename DomNode DominatorTreeNode

chore(ssa refactor): use domtree helper in tests

e483e15

chore(ci): Utilize new workflow to build binaries (#1250)

ed22594

* chore(ci): Utilize new workflow to build binaries * Update release.yml --------- Co-authored-by: Tom French <[email protected]>

chore(noir): Release 0.5.0 (#1202)

0379645

* chore(noir): Release 0.5.0 * chore: Update lockfile

fix: Add Poseidon examples into integration tests (#1257)

4ebeb41

add poseidon examples into nargo_cli

chore(noir): Release 0.5.1 (#1264)

c46f50c

* chore(noir): Release 0.5.1 * chore: Update lockfile

chore: fix clippy warning (#1270)

11943b6

chore(noir): constrain expr; -> assert(expr); (#1276)

dc1522e

* chore(noir): constrain expr; -> assert(expr); * chore(noir): replace remaining `constrain` with `assert(expr)` --------- Co-authored-by: Tom French <[email protected]>

fix: update usage of cfg.pred_iter to cfg.predecessors

f8a8933

joss-aztec closed this May 2, 2023

TomAFrench deleted the joss/domtree branch July 18, 2023 05:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ssa refactor): dominator tree #1253

feat(ssa refactor): dominator tree #1253

joss-aztec commented Apr 28, 2023 •

edited

Loading

kevaundray left a comment

kevaundray Apr 28, 2023

kevaundray Apr 28, 2023

kevaundray Apr 28, 2023

jfecher left a comment

jfecher Apr 28, 2023

jfecher Apr 28, 2023

jfecher Apr 28, 2023

joss-aztec May 2, 2023

jfecher Apr 28, 2023

joss-aztec May 2, 2023

joss-aztec May 2, 2023

jfecher Apr 28, 2023

TomAFrench commented May 2, 2023

joss-aztec commented May 2, 2023

joss-aztec commented May 2, 2023

		@@ -0,0 +1,443 @@
		use std::{cmp::Ordering, collections::HashMap};


		use crate::ssa_refactor::ir::{basic_block::BasicBlockId, function::Function};

		/// DFT stack state marker for computing the cfg post-order.

feat(ssa refactor): dominator tree #1253

feat(ssa refactor): dominator tree #1253

Conversation

joss-aztec commented Apr 28, 2023 • edited Loading

Related issue(s)

Description

Summary of changes

Dependency additions / changes

Test additions / changes

Checklist

Documentation needs

Additional context

kevaundray 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

jfecher 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

TomAFrench commented May 2, 2023

joss-aztec commented May 2, 2023

joss-aztec commented May 2, 2023

joss-aztec commented Apr 28, 2023 •

edited

Loading