Group to Seq #1086

calebmkim · 2022-07-07T20:35:35Z

Transforms a big group into a seq of two smaller groups.
OrderAnalysis is used to determine if we can even apply the transformation.

I can writeup more details here, but part of me thinks it may be better to just discuss details in person.

Edit: Failing bc it's applying the transformation and that is changing some of the the things in the examples/ folder,
and changing the number of cycles for some of the correctness tests.
Edit2: Changed these tests to reflect my changes.

calyx/src/passes/group_to_seq.rs

calebmkim · 2022-07-11T12:42:51Z

Couple of questions about this pass, @rachitnigam.

Should we even try to detect groups that have chains of length > 2-- for example, when 3 registers are being written to in sequence within a group? The code I have right now tries (incorrectly) to detect length > 2, but it seems the Calyx code will have to be quite complicated for these situations. Should I just try to focus on chains of length 2?

(More code specific questions)
2) When I call [ir::Start] and iterate through comp.groups, it seems I still have to eventually replace some of the groups with a seq in the control tree-- if so, how do I do this and if not, what should I do instead?

It seems I iterate through comp.groups mutably in order to be able to get ownership of the assignments of the group using drain(..) and a mutable reference to comp in order to build the ir::Builder (sorry if this is confusing). Any thoughts on how to address this?

rachitnigam · 2022-07-11T18:06:21Z

I think chains of more than 2, as you predict, are going to be hard to detect. You'd have to come up with some way to figure out which registers are being used like an FSM and then use that information to detect what the chain looks like. I think focusing on 2-component chaining is the way to go for now.
Got it! In that case, you can iterate over the groups first and come up with a mapping from Group -> Seq representing all the groups that you need to transform. Then, you can visit the enable and replace the group if it is in the mapping.
Ah, this is a pretty standard problem. ir::Builder lets you directly access the underlying component: builder.component

calebmkim · 2022-07-11T20:57:31Z

@rachitnigam (sorry to bother again)
I am still running into problems using builder

 let mut builder = ...; 
 for group in builder.component.groups.iter_mut(){
    ... 
   builder.add_group(...); 
}

This is giving me an error:
It says I have two mutable borrows: one for buider.components.groups.iter_mut() and one for builder.add_group(..). And it has to be iter_mut since if it isn't then I will have an immutable and a mutable borrow.

Also, are there any other benefits to doing this in the start rather than enable method: your earlier comment said it saves traversing the control tree, but from 2) we will have to do that anyways even if we do it through start? I'm asking mainly bc doing it in start is giving me the problem above.

rachitnigam · 2022-07-12T01:53:29Z

The same group may be used from multiple enable statements:

seq {
  one;
  two;
  one;
}

In this case, you'll end up re-running the analysis for each enable(one).

To iterate over groups, you have to "detach" it from the component:

let groups = component.groups.drain(..).collect();
let builder = ir::Builder(component, ..);
for g in groups { ... }
component.groups = groups;

Does that make sense?

…ons such as weird go asmts

…earlier

calyx/src/analysis/order_analysis.rs

rachitnigam · 2022-07-13T04:11:38Z

calyx/src/analysis/order_analysis.rs

+#[derive(Default)]
+///Primarily used to help determine the order cells are executed within
+///the group
+pub struct OrderAnalysis {


Is this implementing the same logic as InferStaticTiming? If so, we should generalize those two

There is some overlap between, mem_wrt_dep_graph of InferStaticTiming and update_map of OrderAnalysis but there are differences as well and in my opinion it's not worth it to try to generalize. Edit: But maybe generalizing is a good idea. It's just it doesn't seem like it would be on first glance.

calyx/src/analysis/order_analysis.rs

calyx/src/passes/canonical.rs

calyx/src/passes/group_to_seq.rs

rachitnigam · 2022-07-13T04:17:15Z

calyx/src/passes/group_to_seq.rs

+            second_group.borrow_mut().assignments.append(&mut snd_asmts);
+
+            //renaming old_group[done] = ... to new_group[done] = ...
+            let new_done = rename_group_done(


The right way to update a group's done condition is using Group::done_cond_mut instead of manually editing the assignments. The latter is unsafe and will be deprecated

If I create a group using builder.add_group(), does it automatically come with a done assignment? Because when I try to use done_cond_mut it's giving me an error and saying the group has no done condition.

calyx/src/analysis/order_analysis.rs

rachitnigam · 2022-07-17T19:41:34Z

calyx/src/default_passes.rs

                CollapseControl,
                CompileRef, //Must run before 'resource-sharing'.
+                GroupToSeq,


You can run DeadGroupRemoval again if you want in this pipeline

…leaned code

calyx/src/passes/group_to_seq.rs

calyx/src/default_passes.rs

rachitnigam · 2022-07-20T17:31:32Z

calyx/src/default_passes.rs

                CollapseControl,
                CompileRef, //Must run before 'resource-sharing'.
+                GroupToSeq,
                CellShare,


Open an issue saying that we should run CellShare as early as possible, ideally before merge-static-par. This cannot be done currently because it doesn't handle ref cells passed into an invoke: https://docs.calyxir.org/lang/memories-by-reference.html#the-easy-way

calyx/src/passes/group_to_seq.rs

tests/passes/group_to_seq/basic_test.futil

…dated test to reflect this

rachitnigam

Okay, this is looking done! A couple of minor comments about structuring the interface for SplitAnalysis. Address it and merge at your discretion!

rachitnigam · 2022-07-21T13:15:24Z

calyx/src/passes/group_to_seq.rs

+        // Add back the groups we drained at the beginning of this method, but
+        // filter out the empty groups that were split into smaller groups
+        comp.groups.append(groups.into_iter().filter(
+            |group: &ir::RRC<ir::Group>| !group.borrow().assignments.is_empty(),


No need to add the type of group here

rachitnigam · 2022-07-21T13:17:01Z

calyx/src/passes/group_to_seq.rs

+        let mut builder = ir::Builder::new(comp, sigs);
+        for g in groups.iter() {
+            let mut group = g.borrow_mut();
+            match SplitAnalysis::get_split(


It seems like SplitAnalysis returns two groups in the Ok case. What if we make it take the group directly instead of passing the assignments and the name? That way, we also avoid this drain(..).collect() call.

rachitnigam · 2022-07-21T13:18:37Z

examples/futil/simple.expect

@@ -1,18 +1,19 @@
 import "primitives/core.futil";
 import "primitives/binary_operators.futil";
-component main(@go go: 1, @clk clk: 1, @reset reset: 1) -> (@done done: 1) {
+component main<"static"=5>(@go go: 1, @clk clk: 1, @reset reset: 1) -> (@done done: 1) {


Really cool effect of this pass that the component can have its static latency inferred!

rachitnigam · 2022-07-21T13:19:25Z

tests/passes/group_to_seq/dahlia_mult.futil

@@ -0,0 +1,92 @@
+// -p group2seq


Run -p validate before group2seq

rachitnigam · 2022-07-21T13:19:41Z

tests/passes/group_to_seq/no_transform.futil

@@ -0,0 +1,43 @@
+//-p canonicalize -p group2seq


Run validate instead of canonicalize

rachitnigam · 2022-07-21T13:19:59Z

tests/passes/group_to_seq/should_transform.futil

@@ -0,0 +1,43 @@
+//-p canonicalize -p group2seq


validate instead of canonicalize

calebmkim added 13 commits July 6, 2022 09:17

first try at group_to_seq, just detecting when we can use it

c237a3e

small progress on group_to_seq

7f67273

testing

8308789

first (messy) working draft of group2seq

cb1318c

added test cases and refactored code

a7a977b

cleaned code

3f14a74

fixed clippy complaints

57dbc98

changed one line in LiveRangeAnalysis from unreachable! to ()

ea4aa09

undoes most recent commit

03ffbc9

changed ordering of passes to prevent error

e2f2992

deleted .futil file I was using to experiment

9c71e2a

deleted ShareSet analysis-- not needed by this PR

8a7ec7f

removed group2seq as default pass (can change back if need be)

09e7e32

rachitnigam reviewed Jul 8, 2022

View reviewed changes

calyx/src/passes/group_to_seq.rs Outdated Show resolved Hide resolved

calebmkim added 5 commits July 11, 2022 22:02

added case for not done ? 1'd1 and cleaned code

9c70ab9

cleaned code. now handles only chains of 2, but also specific situati…

528d745

…ons such as weird go asmts

added dead_group_removal to default passes-- accidentally deleted it …

d5ad78b

…earlier

overwrote tests to pass group_to_seq as default pass

a55ed6b

fixed clippy complaints

1a7fe93

rachitnigam reviewed Jul 13, 2022

View reviewed changes

calyx/src/analysis/order_analysis.rs Outdated Show resolved Hide resolved

rachitnigam reviewed Jul 13, 2022

View reviewed changes

calyx/src/analysis/order_analysis.rs Outdated Show resolved Hide resolved

rachitnigam reviewed Jul 13, 2022

View reviewed changes

calyx/src/passes/canonical.rs Outdated Show resolved Hide resolved

rachitnigam reviewed Jul 13, 2022

View reviewed changes

calyx/src/passes/group_to_seq.rs Outdated Show resolved Hide resolved

rachitnigam reviewed Jul 13, 2022

View reviewed changes

calyx/src/passes/group_to_seq.rs Outdated Show resolved Hide resolved

rachitnigam reviewed Jul 13, 2022

View reviewed changes

calebmkim added 4 commits July 14, 2022 08:18

cleaned up code

a0cc950

minor changes

f2c2f04

renamed methods and added stable output port requirement

6ea65e1

changed test to pass

9690fe7

rachitnigam reviewed Jul 17, 2022

View reviewed changes

calyx/src/analysis/order_analysis.rs Outdated Show resolved Hide resolved

rachitnigam reviewed Jul 17, 2022

View reviewed changes

calebmkim added 3 commits July 18, 2022 13:06

cleaned code to use fewer if-else stmts

6ca2218

order_analysis no longer in its own file-- split analysis instead + c…

cad12c5

…leaned code

small change

6b09b4d