-
Notifications
You must be signed in to change notification settings - Fork 51
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
Create IDL Backend #1729
Create IDL Backend #1729
Conversation
Some open questions: Naming and placement. This is probably not xilinx specific so should be moved up a level into just src. Also, naming is generic and not good and should be changed TODO: Only works withs d1 memories, but should not be hard to alter
Previously could only handle 1 dimension mems. NOTE: This flattens dimensions into a single size variable that is spit out. However it may be useful to maintain info about shape of memory? Need to think about this more
00988ef
to
729ffdc
Compare
729ffdc
to
5fce9bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good overall! I think my main substantive comment in here is that there are a few pre-existing utility functions that would probably benefit from being moved to their own file so they can be shared between the different backends that use them (to avoid copying-and-pasting the code for the utility functions).
@@ -18,6 +18,7 @@ string-interner.workspace = true | |||
itertools.workspace = true | |||
linked-hash-map.workspace = true | |||
serde = { workspace = true } | |||
serde_json.workspace = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am ignorant enough of Rust workspace setups to not exactly know the difference between using serde_json.workspace = true
here vs. just doing this right here:
serde_json = "1.0"
…and omitting this from the workspace-global Cargo.toml
. Presumably there is a difference but I'm not sure what it is! Maybe this is the right thing; just thought I'd double-check this was the intended strategy (vs. what we do for csv
and vast
below).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe @rachitnigam has an opinion on this? Tbh I've just been pattern matching from stuff I've seen around, the details of these workspaces elude me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably doesn't matter much! I guess the upside of what you have now is that anyone else in the tree who ends up needing serde_json
will use the same version.
calyx-backend/src/xilinx/idl.rs
Outdated
} | ||
|
||
// Returns Vec<String> of memory names | ||
fn external_memories(comp: &ir::Component) -> Vec<String> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like external_memories
is identical to the definition in top-level.rs
. (And get_mem_info
is similar but does the "size flattening" thing internally?) What do you think about moving this stuff to a common utility file so both backends can use it, to avoid the duplication of code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully this is better now? @sampsyo for visibility.
Moved things into calyx-ir/utils. Things like get_mem_info and functions that get memories marked as external, along with their names
6814c63
to
e04c1e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wahoo! Looks great!! Go ahead and hit that green button whenever you are ready. 😃
@nathanielnrn let's get this merged if everything looks good! |
* WIP, add to backend options * add serde_json dependency for IDL backend * WIP work on IDL * Minimal working version of IDL generator Some open questions: Naming and placement. This is probably not xilinx specific so should be moved up a level into just src. Also, naming is generic and not good and should be changed TODO: Only works withs d1 memories, but should not be hard to alter * Let IDL generator handle multi-dim mems Previously could only handle 1 dimension mems. NOTE: This flattens dimensions into a single size variable that is spit out. However it may be useful to maintain info about shape of memory? Need to think about this more * add basic runt tests * clippy formatting * rename idl to yxi and make serde_json call shorter * add error conversion for serde_json for yxi backend * update runt tests for yxi * update cargo lock with serde_json * formatting * move yxi up a directory * WIP of extracting utility functions * Refactor toplevel and yxi util functions Moved things into calyx-ir/utils. Things like get_mem_info and functions that get memories marked as external, along with their names * clippy and formatting
This PR creates a basic backend which takes in a calyx program and outputs information about the program's top level memories.
This is in the name of working on #1105 and #1603.
Specifically, for calyx programs like this
This backend produces
A few things to note that probably warrant further discussion:
@external.
That may be all we need for interfacing with our programs via AXI?*memh
goals and what*memh
even does to understand what information we will need.size
. Would it be useful to retain that information instead?xilinx/
but I'm hoping to get this out tonight for some early feedback and I still struggle with rust module/hierarchy issues, so will address later.EDIT
Some answers to the above after a synch discussion:
@external
memories. Furthermore, only examining the toplevel memories that are external is okay, as marking memories that are parts of subcomponents in a calyx program is a bit messy and probably shouldn't be valid in the first place.*memh
is not. Our wrapper will wire up these*memh
calls as needed to enable simulation.YXI
is our name of choice. Will have.yxi
file extensions and should be moved up a directory.