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

Make inline memory initialization the default #2260

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

Conversation

carlosedp
Copy link
Contributor

Fixes #1293

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you add appropriate documentation in docs/src?
  • Did you state the API impact?
  • Did you specify the code generation impact?
  • Did you request a desired merge strategy?
  • 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?

@sequencer
Copy link
Member

This seems contains a breaking change? There should be a deprecation flow.

@ekiwi
Copy link
Contributor

ekiwi commented Nov 29, 2021

This seems contains a breaking change? There should be a deprecation flow.

Good point. How would you suggest we approach this?

* [[https://github.com/freechipsproject/chisel3/wiki/Chisel-Memories#loading-memories-in-simulation "Loading Memories
* in Simulation"]]
*/
object loadMemoryFromFile {
Copy link
Member

Choose a reason for hiding this comment

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

I didn't see any LoadMemoryTransform here?
Is the file name correct?
I guess loadMemoryFromFile is just a rename from loadMemoryFromFileInline?
This introduce a API breaking change, and seems didn't do anything else?

@sequencer
Copy link
Member

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)

@ekiwi
Copy link
Contributor

ekiwi commented Nov 29, 2021

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.

@ekiwi
Copy link
Contributor

ekiwi commented Nov 30, 2021

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)

@carlosedp
Copy link
Contributor Author

Ah yes, you mean the problem pointed-out by #1289 right?

@sequencer
Copy link
Member

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

@carlosedp
Copy link
Contributor Author

Ping... news about this?

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.

loadMemoryFromFile() should place readmemh() inline for better compatibility
3 participants