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

loadMemoryFromFile doesn't work with memories of aggregate types #968

Closed
nhynes opened this issue Dec 26, 2018 · 3 comments
Closed

loadMemoryFromFile doesn't work with memories of aggregate types #968

nhynes opened this issue Dec 26, 2018 · 3 comments

Comments

@nhynes
Copy link

nhynes commented Dec 26, 2018

Type of issue: bug report

Impact: API addition (no impact on existing code)

What is the current behavior?

Mem of type Bundle is split into single-type memories with names like theMem_field1 and theMem_field2. Unfortunately, the loadMemoryFromFile annotation only contains the top-level name, so when processed by treadle, one receives the error

java.util.NoSuchElementException: key not found: MyModule.theMem

What is the expected behavior?

loadMemoryFromFile should work on memories of aggregate types.

Please tell us about your environment:

master branch of all relevant repos

@nhynes
Copy link
Author

nhynes commented Dec 27, 2018

Okay, so I'm going to try to fix this by adding a firrtl pass which replaces a LoadMemory annotation for a Bundle memories with one LoadMemory for each sub-data.

The fix should definitely happen at the firrtl level since chisel doesn't know that memories are split; and both chisel2verilog and treadle depend on the firrtl annos (meaning that there's a common place to insert the modification).

Writing a compiler pass is motivated by annotations generally not having type information. To add this would also require chisel annotations to know too much about the firrtl IR (i.e. fir.Type). Thus, the only place where types and annotations are seen together is in CircuitState, which is only available to passes.

@nhynes
Copy link
Author

nhynes commented Dec 27, 2018

Turns out the real issue was that chisel-testers2 was passing the old, un-renamed annotations into the ToLoFirrtl pass. This is what I get for using bleeding edge repos!

@jackkoenig
Copy link
Contributor

Thanks for finding and fixing the bug :)

jackkoenig pushed a commit that referenced this issue Feb 28, 2023
This updates the spec to refelect the changes made in #587.
It also fixes issue #968.
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

No branches or pull requests

2 participants