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 dimensional and type data to yxi outputs #1964

Merged
merged 19 commits into from
Mar 23, 2024
Merged

Add dimensional and type data to yxi outputs #1964

merged 19 commits into from
Mar 23, 2024

Conversation

nathanielnrn
Copy link
Contributor

@nathanielnrn nathanielnrn commented Mar 9, 2024

This PR adds memory-type (Sequential or Combinational) data, explicit dimensionality sizes and idx sizes, and makes yxi look for both @external memories and ref memories (before it was limited to just @external memories). #1932

I would be happy to hear any thoughts on the naming of the information output, or if there is something we wanted to be output that is being missed @ayakayorihiro.

Currently a draft because this doesn't compile correctly with a normal cargo build.

This is because the yxi backend is not gated behind any feature flags, but relies on calyx-ir/utils to have access to serde, which is gated behind --features serialize. So I guess we can either gate yxi behind --features serialize or --features yxi or remove the gating of serde in calyx-ir/utils. It might be worth noting that the yxi backend already has a non-gated dependency on serde. Dunno if that's the way things should be.

cc @rachitnigam @EclecticGriffin Any thoughts on best way to proceed with the feature gating issue?

nathanielnrn and others added 5 commits March 5, 2024 11:52
Adds memory_type ((seq or comb), dimension count, dimension sizes, and
total size

TODO: Fix dpeendent import issues
TODO: Test & change runt outputs
Can now build correctly when passing `--features serialize` flag.
Normal compilation does not work
@nathanielnrn nathanielnrn added the C: FPGA Changes for the FPGA backend label Mar 9, 2024
@EclecticGriffin
Copy link
Collaborator

EclecticGriffin commented Mar 11, 2024

wrt feature gating, I think that the simplest option might just be to add an yxi feature (which itself depends on serialization) and so consequently doesn't compile the axi backend if the feature is not given. I think this is reasonable and how other crates handle this type of stuff in practice, but if we don't want to add another feature then I guess the entire axi backend needs to be behind the serialization feature. I suppose they're both identical.

I prefer one of the above over making the axi backend a no-op when the serialization feature is not used since that seems needlessly confusing. Regardless, we don't want to have an ungated dependency on serde since that breaks compilation.

I don't know if @sampsyo has thoughts on this

@rachitnigam
Copy link
Contributor

I think we should eventually move AXI out of thr compiler and mame it a standalone tool

@ayakayorihiro
Copy link
Contributor

ayakayorihiro commented Mar 12, 2024

Sorry for the late reply on this, the new JSON fields are great!! This has all the info I need for my testbench creation script 🙂

@nathanielnrn
Copy link
Contributor Author

I think the dependencies look better now, could either/both of you confirm? @EclecticGriffin @rachitnigam
The yxi backend now only gets compiled with a cargo build --features yxi

@nathanielnrn nathanielnrn marked this pull request as ready for review March 16, 2024 18:40
@nathanielnrn nathanielnrn changed the title WIP: Add dimensional and type data to yxi outputs Add dimensional and type data to yxi outputs Mar 16, 2024
Copy link
Contributor

@rachitnigam rachitnigam left a comment

Choose a reason for hiding this comment

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

Most changes seems fine except the one comment I left. Overall, I'd like for YXI to be a separate tool from the Calyx compiler because it doesn't need the rest of the optimization infrastructure.

Dockerfile Outdated
@@ -54,7 +54,7 @@ WORKDIR /home
RUN git clone https://github.com/cucapra/dahlia.git
WORKDIR /home/dahlia
## Checkout specific version. Fetch before checkout because clone might be cached.
RUN git fetch --all && git checkout 88e05e5
RUN git fetch --all && git checkout master
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't do this. We need to keep the Dahlia version pinned.

@nathanielnrn
Copy link
Contributor Author

Most changes seems fine except the one comment I left. Overall, I'd like for YXI to be a separate tool from the Calyx compiler because it doesn't need the rest of the optimization infrastructure.

Thanks for taking a look.
I think I'd need a bit of guidance on how to start migrating YXI out of the compiler, could maybe talk about this synchronously/on slack? My main concern is the fact that YXI currently relies on the IR to get information about memories. Would YXI just be standalone with dependencies on the IR? Not exactly sure what that looks like in practice.

@nathanielnrn nathanielnrn merged commit 7a2a236 into main Mar 23, 2024
18 checks passed
@nathanielnrn nathanielnrn deleted the yxi-additions branch March 23, 2024 17:08
jiahanxie353 pushed a commit to jiahanxie353/calyx that referenced this pull request May 29, 2024
* Tried to change yxi utils and output

Adds memory_type ((seq or comb), dimension count, dimension sizes, and
total size

TODO: Fix dpeendent import issues
TODO: Test & change runt outputs

* cfgs

* Add crf[...serialize...] in calyx-ir utils.

Can now build correctly when passing `--features serialize` flag.
Normal compilation does not work

* Update runt tests

* Refactoring

* run clippy

* update cargo.tomls to add an yxi feature

* formatting

* change toplevel xilinx backend to correctly get total size of memories

* change dockerfile to build with

* change dahlia checkout to latest commit in master in Dockerfile

* add --all-features to cargo build in .github/workflows/rust.yml

* Revert "change dockerfile to build with"

This reverts commit e71886a.

* revert dockerfile to pinned dahlia checkout

---------

Co-authored-by: Rachit Nigam <rachit.nigam12@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: FPGA Changes for the FPGA backend
Projects
None yet
4 participants