chisel
chisel copied to clipboard
Make inline memory initialization the default
Fixes #1293
Contributor Checklist
- [x] Did you add Scaladoc to every public function/method?
- [x] Did you add at least one test demonstrating the PR?
- [x] Did you delete any extraneous printlns/debugging code?
- [x] Did you specify the type of improvement?
- [x] Did you add appropriate documentation in
docs/src
? - [x] Did you state the API impact?
- [x] Did you specify the code generation impact?
- [x] Did you request a desired merge strategy?
- [x] Did you add text to be included in the Release Notes for this change?
Type of Improvement
- new feature/API
API Impact
This PR changes the default memory initialization based on files by using the inline readmem[bh]
statements instead of the previous SV Bind module and also moves the API to chisel3.util
.
The bind module initialization is available on chisel3.util.experimental
as loadMemoryFromFileBind
.
Backend Code Generation Impact
No changes on backend, just API to generate initialization.
Desired Merge Strategy
- Squash: The PR will be squashed and merged (choose this if you have no preference.
Release Notes
This PR changes the default memory initialization based on external files by using the inline readmem[bh]
statements instead of the previous SV Bind module and also moves the API to chisel3.util
.
Reviewer Checklist (only modified by reviewer)
- [ ] Did you add the appropriate labels?
- [ ] Did you mark the proper milestone (Bug fix:
3.3.x
, [small] API extension:3.4.x
, API modification or big change:3.5.0
)? - [ ] Did you review?
- [ ] Did you check whether all relevant Contributor checkboxes have been checked?
- [ ] Did you mark as
Please Merge
?
This seems contains a breaking change? There should be a deprecation flow.
This seems contains a breaking change? There should be a deprecation flow.
Good point. How would you suggest we approach this?
Seems currently this PR is a draft PR @carlosedp? Sorry I merged master into this(I thought it's done) For that breaking change, I think you can don't touch the original API, and implement your new API, after merging, creating a new deprecating PR to deprecate the API you think it should be removed, then, we can discuss: should we deprecate that API?(I personally like getting that API removed)
For that breaking change, I think you can don't touch the original API, and implement your new API, after merging, creating a new deprecating PR to deprecate the API you think it should be removed, then, we can discuss: should we deprecate that API?(I personally like getting that API removed)
@sequencer : I told @carlosedp to make a PR in order to get the discussion started. Essentially what this PR does is to make a new public API chisel3.util.loadMemoryFromFile
which will use the emitter option to generate an inline readmem call. The old experimental
API is then removed.
While we are talking about this, I just remembered one of the missing features which are needed before we can make "inline" memory loading the default:
- currently there is no way to initialize a memory of a "complex" (i.e. bundle or vector) type, which is something that was supported by the old functionality (albeit in a rather awkward fashion)
Ah yes, you mean the problem pointed-out by https://github.com/chipsalliance/chisel3/issues/1289 right?
I told @carlosedp to make a PR in order to get the discussion started. Essentially what this PR does is to make a new public API chisel3.util.loadMemoryFromFile which will use the emitter option to generate an inline readmem call. The old experimental API is then removed.
I see, I didn't object keeping deprecation flow for experimental
API. I think if @jackkoenig approve this, we should directly merge this ;p
Ping... news about this?