chisel icon indicating copy to clipboard operation
chisel copied to clipboard

Make inline memory initialization the default

Open carlosedp opened this issue 3 years ago • 8 comments

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?

carlosedp avatar Nov 24 '21 19:11 carlosedp

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

sequencer avatar Nov 29 '21 19:11 sequencer

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

Good point. How would you suggest we approach this?

ekiwi avatar Nov 29 '21 19:11 ekiwi

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)

sequencer avatar Nov 29 '21 21:11 sequencer

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 avatar Nov 29 '21 22:11 ekiwi

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)

ekiwi avatar Nov 30 '21 22:11 ekiwi

Ah yes, you mean the problem pointed-out by https://github.com/chipsalliance/chisel3/issues/1289 right?

carlosedp avatar Dec 01 '21 13:12 carlosedp

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

sequencer avatar Dec 01 '21 13:12 sequencer

Ping... news about this?

carlosedp avatar Dec 28 '21 13:12 carlosedp