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

Add bypass register primitive #2030

Closed
wants to merge 1 commit into from
Closed

Add bypass register primitive #2030

wants to merge 1 commit into from

Conversation

matth2k
Copy link
Collaborator

@matth2k matth2k commented Apr 28, 2024

Having this primitive built into the Calyx library would be useful for the AMC memory flow. The use case for this is I have an arbiter de-muxing data from a memory load interface, so I introduced my own inline primitive to latch the data without adding a cycle of latency.

Are there any test cases I should add, besides fixing the ones it broke? Either for parsing sake or an actual use-case? Should the stable attribute on the bypass register be removed?

@andrewb1999
Copy link
Collaborator

I'm not sure that compile.futil is the right place for this primitive since it's not required to compile Calyx programs to verilog. I agree it would be nice to have this somewhere in the primitive library though, I think this pattern will start showing up a lot in dynamic use cases. @rachitnigam are you okay with this being somewhere in the primitive library? And if so, where should it go?

@andrewb1999
Copy link
Collaborator

This is failing some test cases and I'm not really sure why.

@rachitnigam
Copy link
Contributor

Hey @matth2k! Thanks for opening the PR. @andrewb1999 is right: we don't want this in compile.futil since that is packaged into every Calyx file for compilation. I would recommend adding this to core.futil and core.sv instead.

Also, we should definitely add a test to make sure the primitive works. No other frontend uses it so very likely that a change could break it in the future.

Copy link
Contributor

This pull request has been marked as stale due to inactivity.

Copy link
Contributor

This stale PR is being closed because it has not seen any activity in 7 days. If you're planning to continue work on it, please reopen it and mention how to make progress on it.

@github-actions github-actions bot closed this Oct 17, 2024
@matth2k
Copy link
Collaborator Author

matth2k commented Oct 29, 2024

@andrewb1999 Can you push this along? It's still a requirement for using Calyx w/ AMC. That, or can I get a quick rundown of how to add new tests?

@EclecticGriffin
Copy link
Collaborator

EclecticGriffin commented Oct 31, 2024

Hey there happy to help out a bit, if I can! So as Rachit said, it would be better for the primitive to be added to the core.futil and core.sv files (or just the former if you're using the inline version, though for consistency with the existing primitives in the file the latter would be preferable).

Once that's done make a simple correctness test. All this needs to do is use the bypass register to demonstrate that it behaves as expected. An example you can look at is the std-bit-slice test. For your case, I think you would want to demonstrate the combinational path via a write into a comb_mem co-occurring with a write to the bypass register. And then separately write the register value into another memory (or different slot on the same memory) without writing to the register.

As for existing tests breaking, I am unable to offer insight at the moment since the logs don't appear to be retained but happy to take a look if errors re-emerge

@andrewb1999
Copy link
Collaborator

Thanks @EclecticGriffin, I'm working on cleaning this up and making a new PR. I should be able to do it soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants