chore: SSA Refactor IR Review [DO NOT MERGE] #1896

kevaundray · 2023-07-08T09:58:30Z

Description

Problem*

Resolves

Summary*

Documentation

This PR requires documentation updates when merged.
- I will submit a noir-lang/docs PR.
- I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

I have tested the changes locally.
I have formatted the changes with Prettier and/or cargo fmt on default settings.

jfecher · 2023-07-10T19:58:49Z

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

-    /// This map is used to ensure that the ValueId for any given foreign functôn is always
+    /// This map is used to ensure that the ValueId for any given foreign function is always
    /// represented by only 1 ValueId within this function.
+    /// TODO: can we merge this into intrinsics?
    foreign_functions: HashMap<String, ValueId>,


I must have missed this change when it was merged. I don't think we should ever key any of these maps by a String since it is unclear what it represents (a function name? Any arbitrary one, or one of a set of expected names? Are they expected to be extended later? etc).
I'd be in favor of creating a dedicated enum for foreign_functions (oracles? We use oracle elsewhere so we should be consistent in the naming), or merging them into the intrinsic enum.

jfecher · 2023-07-10T19:59:42Z

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

@@ -68,6 +69,7 @@ pub(crate) struct DataFlowGraph {
    /// Debugging information about which `ValueId`s have had their underlying `Value` substituted
    /// for that of another. This information is purely used for printing the SSA, and has no
    /// material effect on the SSA itself.
+    /// TODO: Is this true? How are we doing aliasing?


No longer true, looks like the comment has been stale for a while now. It should be updated.

jfecher · 2023-07-10T20:01:09Z

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

@@ -103,6 +105,8 @@ impl DataFlowGraph {
    /// block's id.
    ///
    /// The pairs are order by id, which is not guaranteed to be meaningful.
+    /// TODO: do we need to order by reverse post order? Is this useful? Check brillig
+    /// TODO: ie do we need to get all successors/predecessors first?


This function is just for when a pass or test needs to iterate through each block and doesn't particularly care about the order (or whether the blocks are even still reachable). For passes that do care about the order, they can create a PostOrder directly like the Dead Instruction Elimination pass does.

jfecher · 2023-07-10T20:02:46Z

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

+        // TODO: Is simplify still inserting the instruction?
+        // TODO: or are we assuming that simplify will always return a ValueId that
+        // TODO: has already been inserted?


Simplify may potentially simplify an instruction to an existing, known value (not necessarily a constant). E.g. add x, 0 is simplified to x. So it is expected simplify will always return a valid ValueId if it returns SimplifyResult::SimplifiedTo.

jfecher · 2023-07-10T20:04:19Z

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

+    ///
+    /// TODO: This is doing more than just inserting an instruction and its results


Sounds like the missing piece is that this method attempts to simplify the instruction to another value and only inserts it if it could not do so. We should:

Update the doc comment above to note this.

Potentially rename the function? I'm unsure what a clearer name would be.

crates/noirc_evaluator/src/ssa_refactor/ssa_builder/mod.rs

jfecher · 2023-07-10T20:53:27Z

crates/noirc_evaluator/src/ssa_refactor/ssa_builder/mod.rs

-    pub(crate) fn import_intrinsic(&mut self, name: &str) -> Option<ValueId> {
-        Intrinsic::lookup(name).map(|intrinsic| self.import_intrinsic_id(intrinsic))
-    }


Why are we removing this function? It looks like it makes codegen_ident more complex.

jfecher · 2023-07-10T20:55:08Z

crates/noirc_evaluator/src/ssa_refactor/ssa_gen/program.rs

        let mut max_id = main_id;

+        // Fetch the max_id from all functions


This map just maps each function id to the function it refers to, keeping track of the max_id is separate.

jfecher · 2023-07-10T20:57:04Z

crates/noirc_evaluator/src/ssa_refactor/ssa_gen/program.rs

+        // TODO: why is this sorting being done here?
+        // TODO: does the max_id need to be the main_id initially
+        // TODO or do generally just need the max_id from all functions?
        let mut max_id = main_id;


There's no sorting being done here, we're just converting the Vec of functions into a btree_map so we can index the map by the function id of each function.

max_id isn't any specific id, it's just used to keep track of what the next free id would be in case we need to add a function to this Ssa struct later and that function needs a fresh ID not shared by existing functions. That is why we initialize next_id: AtomicCounter::starting_after(max_id) later on.

jfecher · 2023-07-10T20:59:17Z

crates/noirc_evaluator/src/ssa_refactor/ssa_gen/program.rs

+    /// TODO: The above condition is somewhat hidden in the API, should we change it so that
+    /// TODO the caller needs to pass in the main function?


Possibly, I'm not sure of a clean way to do so however. I think we'd be relying on users to manually extract main from a Vec only to have this function re-insert it afterward. The FunctionBuilder is the only real user of this function I believe so another option is to restrict the visibility a bit more.

kevaundray · 2023-07-10T21:06:58Z

(Note, this will never be merged)

jfecher · 2023-08-15T20:18:01Z

Closing this PR as it was not meant to be merged

ir review

c43c179

jfecher requested changes Jul 10, 2023

View reviewed changes

jfecher closed this Aug 15, 2023

TomAFrench deleted the kw/add-comments branch November 20, 2024 12:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: SSA Refactor IR Review [DO NOT MERGE] #1896

chore: SSA Refactor IR Review [DO NOT MERGE] #1896

kevaundray commented Jul 8, 2023

jfecher Jul 10, 2023

jfecher Jul 10, 2023

jfecher Jul 10, 2023

jfecher Jul 10, 2023

jfecher Jul 10, 2023

jfecher Jul 10, 2023

jfecher Jul 10, 2023

jfecher Jul 10, 2023

jfecher Jul 10, 2023

kevaundray commented Jul 10, 2023

jfecher commented Aug 15, 2023

		///
		/// TODO: This is doing more than just inserting an instruction and its results

		let mut max_id = main_id;

		// Fetch the max_id from all functions

		/// TODO: The above condition is somewhat hidden in the API, should we change it so that
		/// TODO the caller needs to pass in the main function?

chore: SSA Refactor IR Review [DO NOT MERGE] #1896

chore: SSA Refactor IR Review [DO NOT MERGE] #1896

Conversation

kevaundray commented Jul 8, 2023

Description

Problem*

Summary*

Documentation

Additional Context

PR Checklist*

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

kevaundray commented Jul 10, 2023

jfecher commented Aug 15, 2023