Reimplement static-timing pass.

[mikeurbach]

Supersedes #908. Closes #654.

[mikeurbach]

Alright, here we go. Please see original discussion from #905.

[rachitnigam]

Okay, housekeeping comments are done. I'm going to read through the final logic once and see if there is anything to change. Just to make sure I remember, I need to:

• check while + @bound works as expected
• seq transition logic can be simplified (question from the original PR)

[rachitnigam]

seq transition logic can be simplified (question from the original PR)

I'm okay with the seq logic as it is. I think there needs to be some redundancy with that logic somewhere; tdcc does something similar with enables I think.

[mikeurbach]

@rachitnigam I think this should be good for review, the housekeeping items should be addressed. What's the best way to reduce duplication in the start_* methods? I tried to write a generic version that just called "calculate_states", but I'm not sure how to get the attributes out of a generic ir::Control.

[rachitnigam]

You can use the GetAttributes trait which is implemented by Control nodes.

[mikeurbach]

Sorry for the noob question, but I am trying to write a generic function that takes a &mut ir::Control, and pass it a &mut ir::Seq, for example. How do you call such a function? It looks like:

fn start_generic(
    con: &mut ir::Control,
    comp: &mut ir::Component,
    sigs: &LibrarySignatures,
    dump_fsm: bool,
) -> VisResult {
...

And I want to say something like:

    fn start_seq(
	&mut self,
        con: &mut ir::Seq,
        comp: &mut ir::Component,
	sigs: &LibrarySignatures,
        _comps: &[ir::Component],
    ) -> VisResult {
        start_generic(con, comp, sigs, self.dump_fsm)
    }

This doesn't work, so I'm trying to "upcast" the &mut ir::Seq to a &mut ir::Control. I tried something like this, but that tries to move out of a mutable reference:

start_generic(&mut ir::Control::Seq(*con), comp, sigs, self.dump_fsm)

[rachitnigam]

Hm, generally you’d have to do this by defining start generic on some shared trait implemented by all the control nodes but there isn’t such a thing. Additionally, since you’re going to dispatch on the input anyways, it might not be useful after all.

maybe it’ll help if you explain which boilerplate you’re trying to remove and I can try to explain a Rust-y way to do it.

[mikeurbach]

Well, all of the start_seq, start_par, etc. all have the same structure, but dispatch to the specific _calculate_states methods. I was thinking it would be good to write a single method with the same structure, which calls the generic calculate_states. start_seq and friends could just call the generic method with their specific control construct.

[rachitnigam]

I see, I'd probably separate the checking logic into a function that takes something that implements GetAttributes:

fn check_static<C: GetAttributes>(con: C) -> bool {
  ...
}

I don't currently have an obvious way to take something like this and also dispatch it. One probable way is using dynamic trait objects (dyn impl) but that forces Rust to build a virtual table for the function which might be unnecessary in this case.

[rachitnigam]

It occurs to me that there might be a simple optimization possible in the realize_schedule method to calculate consecutive "runs" of FSM states where we basically just want fsm.in = fsm.out + 1 and simplify the generated transitions a bit. Not super crucial but will probably start mattering when you look at resources.

[mikeurbach]

Opened an issue for this enhancement so we don't lose it: #929.

[rachitnigam]

LGTM! Thanks for bearing with the back and forth @mikeurbach and super awesome work on this. I've felt a comment on a simple FSM optimization that could be useful but I'll leave it to your judgement. Merge when you think this is ready and feel free to ask any more questions.

[mikeurbach]

I don't currently have an obvious way to take something like this and also dispatch it.

If you're fine with this level of duplication, so am I. The checking could be factored out, but it's not going to save much.

I've felt a comment on a simple FSM optimization that could be useful but I'll leave it to your judgement

I was thinking about this a bit as well. In a lot of cases I think we would be able to have the FSM state simply increment.

I'm inclined to leave this be for now, and consider this a starting point for more optimizations. This currently provides the behavior I'm looking for, so I'm happy if further resource reductions are added later, along with #911 and other such optimizations.

[mikeurbach]

Thanks for all the iterations on this @rachitnigam! Landing this now as a starting point, and we can tackle the further enhancements in smaller PRs now that the functionality is locked in.