[Calyx] Add Static Groups and Control #5354
Conversation
added static par op
Static Seq
There was a problem hiding this comment.
Overall LGTM.
From an engineering perspective, I "think" we could probably reduce code duplication by having interfaces IfOpInterface, SeqOpInterface, ParOpInterface. Then have StaticSeqOp and DynamicSeqOp (or whatever). WDYT? Maybe you already tried this?
Edit: also thanks for this work! Very cool :-)
| @@ -0,0 +1,188 @@ | |||
| // RUN: circt-translate --export-calyx --split-input-file --verify-diagnostics %s | FileCheck %s --strict-whitespace | |||
There was a problem hiding this comment.
Can you try round-tripping with the native compiler to confirm this works as expected?
https://docs.calyxir.org/fud/circt.html#native-representation-to-mlir
There was a problem hiding this comment.
I don't think I can do this in the CIRCT CI currently. As far as I know the Calyx native compiler is not included as an optional dependency of CIRCT. I think there's a decent argument that it should be, but that's beyond the scope of this PR.
There was a problem hiding this comment.
This can't be done because we don't live in the world of a monorepo.
So, unfortunately, we need to manually test this. I'm asking if you can ensure that your new changes round trip with the native compiler, i.e., does the native compiler emit correctly emit your new dialect additions. If it is not the case, then please open a bug in the native Calyx GitHub.
There was a problem hiding this comment.
Oh ok, I understand what you mean now. I will test this manually.
There was a problem hiding this comment.
It doesn't seem to work directly. I will open an issue on the native Calyx repo.
There was a problem hiding this comment.
I took a quick look, and left some minor comments. Also agree with @cgyurgyik's suggestions.
Thanks for adding this, I'm excited to see these representations in CIRCT!
| }]; | ||
| let arguments = (ins | ||
| I32Attr:$start, | ||
| OptionalAttr<I32Attr>:$end |
There was a problem hiding this comment.
For repeat and cycle, are 32 bits enough? I guess probably...
There was a problem hiding this comment.
Yeah I made a guess here that 32 bits would be enough. I guess it's not a big deal to switch to 64 bits, but I'm not sure if it matters for any reasonable design.
|
Ok I added an interface for the If ops and addressed the other comments. I'm not sure that interfaces make sense for Seq and Par ops currently because most of the shared code is already handled by the ControlLike interface. I'm pretty sure that with the current Seq and Par ops adding an interface would introduce more code than it removes, but the interface could still make sense in the future if our uses of Seq and Par change a bit. Let me know what you think about those interfaces, but I think it's not really useful to have them now. |
|
Sorry i haven't had a chance to take a second look, I will come back and review this. |
|
Does anyone have a chance to review this further soon? I think I have addressed the issues and not having this merged is starting to block me. |
| @@ -0,0 +1,188 @@ | |||
| // RUN: circt-translate --export-calyx --split-input-file --verify-diagnostics %s | FileCheck %s --strict-whitespace | |||
|
I'm going to go ahead and merge this now, but if anyone has any extra comments in the future I will be happy to address them in another PR. |
Co-authored-by: Pai Li <pl582@cornell.edu> Co-authored-by: Caleb <cmk265@cornell.edu>
Adds Calyx static groups and control as proposed here. Static groups and control provide a first-class way to ensure the native compiler has enough information to produce efficient, static FSMs.
The first use for these static ops within CIRCT will be improving the LoopScheduleToCalyx pass, as we can now rely on static behavior to generate efficient loop FSMs.