Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Static Groups #1403

Merged
merged 27 commits into from
Apr 7, 2023
Merged

Implement Static Groups #1403

merged 27 commits into from
Apr 7, 2023

Conversation

calebmkim
Copy link
Contributor

Very basic implementation of static groups. Syntax looks like

static group<4> foo {
    mult.go = %[2:3] ? 1'd1 
    r.write_en = %1 ? 1'd1 
    ...
    foo[done] = %4? 1'd1 // this must match the given latency <4> 
} 

Some notes:

  • It currently creates a new fsm for each static group, which is obviously bad for performance
  • Group2Seq is not enabled for static groups. Mainly since I'm not sure what latency to give the static groups once we have split them up. Seems like Group2Seq needs to do a different sort of analysis for StaticGroups when we split them up so we have that information.
  • I've implemented some of the feedback from Added Static Assignments to IR  #1402, but the ones that I think will take a bit longer it'll prob be better to do in a different PR, so I just added them to the tracker issue To-Do Tasks for Static Groups  #1397.

@rachitnigam
Copy link
Contributor

Über excited for this!!! Couple of questions

  1. Is %[2:3] the same as %2, i.e. is the interval half-open on the right side?
  2. The group2seq limitation makes sense to me. In the long term, we'd want to make it apply to only static groups since being able to do the transformation depends on timing information. You're right that the question of how to give timing information to the generated groups is funky but you can think of this pass as being the inverse of the "smart static group compilation" pass that we've talked about–it tries to turn groups into static seqs
  3. Sounds good!

@calebmkim
Copy link
Contributor Author

Intervals are closed on both sides for right now (i.e., %[2,2] = %2), although it would be quite easy to change if we wanted to.

@rachitnigam
Copy link
Contributor

We should probably make them exclusive on the right side since people seem to expect end-exclusive ranges

@rachitnigam
Copy link
Contributor

rachitnigam commented Apr 4, 2023

One very syntactic nitpick: The syntax foo[done] = %4? 1'd1 is a little too general–it implies that maybe I can put something else in the place of 1'd1. Similarly, the information encoded in static group<4> foo redundant. Is there a way to switch to just using foo[done] = %4 and not have the group<n> syntax at all?

Also, long term, we should convert the tdst correctness tests to just be static group correctness tests since static groups are the things that provide static timing guarantees. What's the current testing story? I saw there are a couple of tests but I'm not sure if we're carefully counting the exact number of cycles they should take and seeing if they maintain the timing invariants.

Apart from these two things, LGTM!

Self {
name,
assignments: vec![],
holes: smallvec![],
attributes: Attributes::default(),
latency,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would've expected StaticGroups to not have a holes field whatsoever since the latency field tells you what the generated done condition should look like

Copy link
Contributor Author

@calebmkim calebmkim Apr 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think you're right. For go holes, I realize that we probably don't need them at all. For done holes, this relates do the point you made about the redundancy of static group g<4> and g[done] = %4 ? 1'd1, since they're really saying the same thing.
To your point on redundancy, I currently make the well-formed pass check that the assignment to g's done hole is exactly g[done] = latency ? 1'd1.

It might be helpful so the group has no done holes: instead, it just has static group g<4> to tell the static latency, and there is no assignment at all that says g[done] = .... This would make our syntax even more specific.

@calebmkim
Copy link
Contributor Author

Right now, since the groups static groups are directly being turned into dynamic groups, we still have "wasted cycles" between groups.

@rachitnigam
Copy link
Contributor

Yeah, that's what I thought too

@calebmkim calebmkim merged commit 8b8b149 into master Apr 7, 2023
@calebmkim calebmkim deleted the impl-static-groups branch April 7, 2023 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants