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

eDSL integrations for pipelined operations #2141

Merged
merged 11 commits into from
Jun 18, 2024
Merged

eDSL integrations for pipelined operations #2141

merged 11 commits into from
Jun 18, 2024

Conversation

KabirSamsi
Copy link
Collaborator

@KabirSamsi KabirSamsi commented Jun 12, 2024

Previously, the calyx-py eDSL has contained functionality for op_store_in_reg, a function which can create any binary combinational cell (i.e. add, le), take in two provided inputs and write the result to a provided register. However, for non-combinational operations involving cells like std_mult_pipe and std_div_pipe, a similar shorthand doesn't yet exist in the eDSL.

This PR attempts to implement that functionality. Directly below the family of op_store_in_reg functions, I have implemented pipe_store_in_reg, which functions very similarly but allows for a non-combinational circuit to complete fully.

Also included are two functions which utilize this – mult_store_in_reg and div_store_in_reg. The structure of these follows the structure of functions like add_store_in_reg, which similarly derive from op_store_in_reg.

I have tested it with a handful of programs locally.

@rachitnigam
Copy link
Contributor

This sounds like a great idea! One caveat: the multiplier primitive (std_mult_pipe) actually "latches" the output which means once it computes the output, it keeps it stored there till the next computation begins. Because of that, you don't actually need to store the multiplier's result in a register.

@KabirSamsi
Copy link
Collaborator Author

Great to know, thanks! Do you recommend I redesign it to avoid storing the result in a new register?

@anshumanmohan
Copy link
Contributor

anshumanmohan commented Jun 12, 2024

That's a nice point, thanks Rachit! That said, I still think this little set of utilities will be nice to have around. For the version that takes advantage of latching, we can do up something like mult_use(a, b). It would:

  • add a multiplier cell to the cells section
  • create a non-combinational group that feeds a and b into the multiplier
  • return handles to the group and the cell

So then then the user is free to run the group and read cell.out, thereby saving a register and reads and writes to it.

@KabirSamsi
Copy link
Collaborator Author

I think we can make use of the binary_use_names function I was discussing with you yesterday @anshumanmohan – to implement mult_use and the such.

Copy link
Contributor

@anshumanmohan anshumanmohan left a comment

Choose a reason for hiding this comment

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

Cool stuff! I have one easy catch, plus one more serious request in the comments below. There is also a potentially more complicated side-quest if you'd like it, but I'm happy to merge without that one.

calyx-py/calyx/builder.py Outdated Show resolved Hide resolved
calyx-py/calyx/builder.py Outdated Show resolved Hide resolved
calyx-py/calyx/builder.py Outdated Show resolved Hide resolved
@anshumanmohan
Copy link
Contributor

I think we can make use of the binary_use_names function I was discussing with you yesterday @anshumanmohan – to implement mult_use and the such.

I think binary_use itself will be able to handle it. As I suggest above, you can either take an is_comb parameter from the user or infer it yourself.

I did a grep and found that binary_use_names is actually used, in frontends/ntt-pipeline/gen-ntt-pipeline.py. I think we can leave it around for that very specific use-case but otherwise let's not get too dependent on user-provided string names.

@KabirSamsi
Copy link
Collaborator Author

KabirSamsi commented Jun 13, 2024

In the latest commits, I've added functionalitry for mul_use and div_use (by putting is_comb checkers inside binary_use), and have refactored the code a fair bit so that redundant code isn't duplicated.

@anshumanmohan If this seems fine by you, I think I'm ready to merge.

Copy link
Contributor

@anshumanmohan anshumanmohan left a comment

Choose a reason for hiding this comment

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

Great stuff, the is_comb method on cells is going to prove very useful I'm sure. I have one little thing in there where I have asked for outside help to try and neaten that method even more, basically following the plan you told me in person. Let's give the team until the end of the day to respond. You can merge after that!

@@ -1335,6 +1383,22 @@ def is_primitive(self, prim_name) -> bool:
isinstance(self._cell.comp, ast.CompInst)
and self._cell.comp.id == prim_name
)

def is_comb(self) -> bool:
return self._cell.comp.id in (
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah so I see you were forced to go with the silly way, by checking cell IDs. Just paging @rachitnigam or @calebmkim for a consult on Kabir's alternate plan. Kabir reasoned that combinational groups lack go and done ports while non-combinational ports have those ports, so he could make an is_comb method on cells by checking whether the cell passed has go and done. Any thoughts about making this work?

My guess as to why this didn't work is this:
The go and done ports do indeed feature in non-comb cells, but those two ports actually get added lower down in the compilation process. So if we look at the eDSL level for those ports, we won't find them. Kabir does that sound about right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that's right – I do have to test it a bit more, but what seems to happen is the following:

At an eDSL level, all cells (combinational or not) have go and done ports. In fact, without the is_comb checker, it's easily possible to compile to erroneous Calyx code as follows:

Screenshot 2024-06-17 at 10 56 35 AM

I have a feeling this may have something to do with the class hierarchy of cell objects in the eDSL, which may be worth looking into separately.

Copy link
Contributor

Choose a reason for hiding this comment

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

Go ahead and merge, Kabir! We'll make it pretty later if we get some insight from the team.

Copy link
Collaborator Author

@KabirSamsi KabirSamsi Jun 18, 2024

Choose a reason for hiding this comment

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

Sounds good, thanks!
I'll leave this conversation open for the time being though.

@KabirSamsi KabirSamsi merged commit 76cc300 into main Jun 18, 2024
18 checks passed
@KabirSamsi KabirSamsi deleted the pipe-store-dsl branch June 18, 2024 13:26
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.

3 participants