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

Multidimensional Memory in calyx-py #2240

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Multidimensional Memory in calyx-py #2240

wants to merge 3 commits into from

Conversation

KabirSamsi
Copy link
Collaborator

This is a little abstraction that I thought could prove useful for the eDSL.

We have 1D memory and 2D memory, both in pure Calyx and in calyx-py. However, I thoguht it may be interesting to integrate multidimensional memories, provided there was a clean way to represent them in Calyx.

This version represents any 'n-d memory block' as a 1-dimensional memory cell, and introduces a simple Python function for translating a series of indices (e.g. mem_rep[i][j][k]) into a corresponding index into the 1d representation. This support extends to both sequential and combinational memory.

@KabirSamsi KabirSamsi changed the title Multidimensional Memory in Calyx Multidimensional Memory in calyx-py Jul 31, 2024
load_grp.done = reg.done
return load_grp

def mem_latch_dn(self, mem, dims, indices, groupname):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if I'm understanding this correctly, this requires indices to be constants at compile-time. If so, it would be useful to add an assert to check that all the indices are in fact fact and throw an error if they are not.

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.

I presume you have a use-case for this! In general I think that an actual, say, 4D memory will be faster than your abstracted version. Is that fair @rachitnigam?

I have reviewed this for code quality etc., but you may also want to surface it on Zulip and ask what folks think! Unless of course you have a motivating example already.

@@ -446,6 +446,24 @@ def comb_mem_d2(
is_external,
is_ref,
)

def comb_mem_dn(
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider a more descriptive name. dn reads like "down" to me! Also flesh out the docstring to explain what's up.

self,
name: str,
bitwidth: int,
lens: List[int],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
lens: List[int],
lengths: List[int],

@@ -482,6 +500,24 @@ def seq_mem_d2(
is_ref,
)

def seq_mem_dn(
Copy link
Contributor

Choose a reason for hiding this comment

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

See above

self,
name: str,
bitwidth: int,
lens : List[int],
Copy link
Contributor

Choose a reason for hiding this comment

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

See above

@@ -898,6 +934,48 @@ def mem_latch_d2(self, mem, i, j, groupname):
latch_grp.done = mem.done
return latch_grp

def flatten_idx(self, dims, indices):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this be private?


def mem_load_dn(self, mem, dims, indices, reg, groupname):
"""Inserts wiring into `self` to perform `reg := mem[i1][i2]...[in]`,
where `mem` is a seq_dn memory or a comb_mem_d1 memory
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, also explain the use-case of how indices has to be used. And consider the assertion that Rachit suggests below!

@rachitnigam
Copy link
Contributor

@anshumanmohan when mapping onto hardware, there is no concept of multidimensional memories so the indexing logic will need to be generated by the synthesis tools (see #907); we are just making that explicit. I would suggest that in the case of power-of-two sizes, we generate optimized logic to compute the single-dimensional index that uses shift-right instead of multiplication.

@anshumanmohan
Copy link
Contributor

Ah gotcha. And yes agreed that it would be good to exploit shifting when possible. Does this all make sense @KabirSamsi? Catch me in person if you’d like to discuss further :)

@KabirSamsi
Copy link
Collaborator Author

Thanks for the feedback – I think I follow! I'll follow up in-person with Anshuman to clarify.

@anshumanmohan
Copy link
Contributor

Just a gentle nudge putting this back on your radar, @KabirSamsi. I think there are a couple concrete things to do here and then we can look at it again!

@KabirSamsi
Copy link
Collaborator Author

Oh my mistake, I thought we had put this on ice. I'll go through Rachit's advice and get back on this!

@rachitnigam
Copy link
Contributor

rachitnigam commented Oct 26, 2024

@KabirSamsi I'm going to go ahead and mark this as stale. If you're intending on working on this, please remove the tag and make the changes. Github will automatically close this PR in 7 days.

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