Skip to content
This repository has been archived by the owner on Aug 20, 2024. It is now read-only.

Add memory initialization options for synthesis #2166

Merged
merged 3 commits into from
Apr 1, 2021

Conversation

carlosedp
Copy link
Contributor

@carlosedp carlosedp commented Mar 31, 2021

This PR adds options for memory initialization inside or outside the
ifndef SYNTHESIS block.

Fixes #2114.

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you update the FIRRTL spec to include every new feature/behavior?
  • 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 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 new feature adds memory initialization options for having the readmem blocks inside or outside the existing ifndef SYNTHESIS.

In the future, this parameter can be used by the -target:fpga PR (#2111) that is in development.

Backend Code Generation Impact

This changes the generated Verilog code based on the parameters:

If using MemoryNoSynthInit (current default):

...
initial begin
  `ifdef RANDOMIZE
    `ifdef INIT_RANDOM
      `INIT_RANDOM
    `endif
    `ifndef VERILATOR
      `ifdef RANDOMIZE_DELAY
        #`RANDOMIZE_DELAY begin end
      `else
        #0.002 begin end
      `endif
    `endif
  `endif // RANDOMIZE
  $readmemh("sample.hex", mem);
end // initial
`ifdef FIRRTL_AFTER_INITIAL
`FIRRTL_AFTER_INITIAL
`endif
`endif // SYNTHESIS
endmodule

If using MemorySynthInit:

initial begin
  `ifdef RANDOMIZE
    `ifdef INIT_RANDOM
      `INIT_RANDOM
    `endif
    `ifndef VERILATOR
      `ifdef RANDOMIZE_DELAY
        #`RANDOMIZE_DELAY begin end
      `else
        #0.002 begin end
      `endif
    `endif
  `endif // RANDOMIZE
end // initial
`ifdef FIRRTL_AFTER_INITIAL
`FIRRTL_AFTER_INITIAL
`endif
`endif // SYNTHESIS
  initial begin
    $readmemh("sample.hex", mem);
  end
endmodule

Desired Merge Strategy

  • Squash: The PR will be squashed and merged (choose this if you have no preference.

Release Notes

This PR adds two annotations to determine how readmem statements are generated in Verilog code.
By default, it is placed inside the ifndef SYNTHESIS block which gets ignored by some FPGA tools.
One can add either MemorySynthInit or MemoryNoSynthInit (which is the default if not defined) annotation to change the behaviour and have the readmem outside this ifndef block.

In the future, MemorySynthInit could be set as default for FPGA targets (-target:FPGA).

TBD

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels?
  • Did you mark the proper milestone (1.2.x, 1.3.0, 1.4.0) ?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you mark as Please Merge?

@carlosedp
Copy link
Contributor Author

carlosedp commented Mar 31, 2021

I'd like some feedback on this approach and if it's the correct way to do it.

I successfully got the results by changing the parameter on def initValue: MemoryInitEmissionValue = MemoryNoSynthInit but don't know how to make it change based on different generation parameters (like the one currently in dev in PR #2111.

Also would be great to have a way to use this until the FPGA target is done.

I thought about something like an annotation:

  annotate(new ChiselAnnotation {
    override def toFirrtl =
      firrtl.MemoryInitEmissionOption = firrtl.MemorySynthInit
  })

But can't figure out how :)

This PR adds options for memory initialization inside or outside the
`ifndef SYNTHESIS` block.
@carlosedp
Copy link
Contributor Author

I think is is much cleaner and allows defining the behaviour by using an annotation in the Chisel code.

  annotate(new ChiselAnnotation {
    override def toFirrtl =
      firrtl.annotations.MemorySynthInit
      // or
      // firrtl.annotations.MemoryNoSynthInit
  })

  val mem = SyncReadMem(words, UInt(bitWidth.W))
  if (memoryFile.trim().nonEmpty) {
    println(s"  Load memory file: " + memoryFile)
    loadMemoryFromFileInline(mem, memoryFile)
  }

@carlosedp carlosedp marked this pull request as ready for review April 1, 2021 00:38
@carlosedp carlosedp force-pushed the readmem-synth branch 2 times, most recently from 52596d8 to 55b3167 Compare April 1, 2021 01:28
@carlosedp
Copy link
Contributor Author

Also I tested that the MemorySynthInit/MemoryNoSynthInit annotation doesn't need to be in the memory module itself. It can be in the Topmodule for example and works fine.

@carlosedp carlosedp requested a review from ekiwi April 1, 2021 15:12
@jackkoenig jackkoenig added this to the 1.4.x milestone Apr 1, 2021
@jackkoenig jackkoenig dismissed ekiwi’s stale review April 1, 2021 21:54

Changes were made

@jackkoenig jackkoenig merged commit d0d3cd4 into chipsalliance:master Apr 1, 2021
mergify bot pushed a commit that referenced this pull request Apr 1, 2021
This PR adds options for memory initialization inside or outside the
`ifndef SYNTHESIS` block.

(cherry picked from commit d0d3cd4)
@mergify mergify bot added the Backported This PR has been backported to marked stable branch label Apr 1, 2021
mergify bot added a commit that referenced this pull request Apr 8, 2021
This PR adds options for memory initialization inside or outside the
`ifndef SYNTHESIS` block.

(cherry picked from commit d0d3cd4)

Co-authored-by: Carlos Eduardo <[email protected]>
Co-authored-by: Jack Koenig <[email protected]>
johnsbrew added a commit to johnsbrew/firrtl that referenced this pull request Jan 21, 2022
- Fix & test MemorySynthInit behavior with MemoryArrayInitAnnotation and MemoryScalarInitAnnotation.
Add test case for MemoryRandomInitAnnotation which is, on the contrary, expected not to leak any randomization statement in synthesis context.

- Refactor MemoryInitSpec for improved results readability

Context:

PR chipsalliance#2166 (commit: 4530152) introduced MemorySynthInit annotation to control whether statement generated with Memory*InitAnnotation (emitted within initial begin block in verilog) should be guarded with ifndef SYNTHESIS or not.
Unfortunately only one configuration (MemoryFileInlineAnnotation) has been tested while the others have been generating incorrect verilog statements (MemoryArrayInitAnnotation and MemoryScalarInitAnnotation).

Signed-off-by: Jean Bruant <[email protected]>
@johnsbrew johnsbrew mentioned this pull request Jan 21, 2022
14 tasks
johnsbrew added a commit to johnsbrew/firrtl that referenced this pull request Jan 21, 2022
- Fix & test MemorySynthInit behavior with MemoryArrayInitAnnotation and MemoryScalarInitAnnotation.
Add test case for MemoryRandomInitAnnotation which is, on the contrary, expected not to leak any randomization statement in synthesis context.

- Refactor MemoryInitSpec for improved results readability

Context:

PR chipsalliance#2166 (commit: 4530152) introduced MemorySynthInit annotation to control whether statement generated with Memory*InitAnnotation (emitted within initial begin block in verilog) should be guarded with ifndef SYNTHESIS or not.
Unfortunately only one configuration (MemoryFileInlineAnnotation) has been tested while the others have been generating incorrect verilog statements (MemoryArrayInitAnnotation and MemoryScalarInitAnnotation).

Signed-off-by: Jean Bruant <[email protected]>
jackkoenig pushed a commit that referenced this pull request Jan 27, 2022
- Fix & test MemorySynthInit behavior with MemoryArrayInitAnnotation and MemoryScalarInitAnnotation.
Add test case for MemoryRandomInitAnnotation which is, on the contrary, expected not to leak any randomization statement in synthesis context.

- Refactor MemoryInitSpec for improved results readability

Context:

PR #2166 (commit: 4530152) introduced MemorySynthInit annotation to control whether statement generated with Memory*InitAnnotation (emitted within initial begin block in verilog) should be guarded with ifndef SYNTHESIS or not.
Unfortunately only one configuration (MemoryFileInlineAnnotation) has been tested while the others have been generating incorrect verilog statements (MemoryArrayInitAnnotation and MemoryScalarInitAnnotation).

Signed-off-by: Jean Bruant <[email protected]>
mergify bot pushed a commit that referenced this pull request Jan 27, 2022
- Fix & test MemorySynthInit behavior with MemoryArrayInitAnnotation and MemoryScalarInitAnnotation.
Add test case for MemoryRandomInitAnnotation which is, on the contrary, expected not to leak any randomization statement in synthesis context.

- Refactor MemoryInitSpec for improved results readability

Context:

PR #2166 (commit: 4530152) introduced MemorySynthInit annotation to control whether statement generated with Memory*InitAnnotation (emitted within initial begin block in verilog) should be guarded with ifndef SYNTHESIS or not.
Unfortunately only one configuration (MemoryFileInlineAnnotation) has been tested while the others have been generating incorrect verilog statements (MemoryArrayInitAnnotation and MemoryScalarInitAnnotation).

Signed-off-by: Jean Bruant <[email protected]>
(cherry picked from commit 475c165)

# Conflicts:
#	src/main/scala/firrtl/backends/verilog/VerilogEmitter.scala
mergify bot pushed a commit that referenced this pull request Jan 27, 2022
- Fix & test MemorySynthInit behavior with MemoryArrayInitAnnotation and MemoryScalarInitAnnotation.
Add test case for MemoryRandomInitAnnotation which is, on the contrary, expected not to leak any randomization statement in synthesis context.

- Refactor MemoryInitSpec for improved results readability

Context:

PR #2166 (commit: 4530152) introduced MemorySynthInit annotation to control whether statement generated with Memory*InitAnnotation (emitted within initial begin block in verilog) should be guarded with ifndef SYNTHESIS or not.
Unfortunately only one configuration (MemoryFileInlineAnnotation) has been tested while the others have been generating incorrect verilog statements (MemoryArrayInitAnnotation and MemoryScalarInitAnnotation).

Signed-off-by: Jean Bruant <[email protected]>
(cherry picked from commit 475c165)
mergify bot added a commit that referenced this pull request Jan 27, 2022
- Fix & test MemorySynthInit behavior with MemoryArrayInitAnnotation and MemoryScalarInitAnnotation.
Add test case for MemoryRandomInitAnnotation which is, on the contrary, expected not to leak any randomization statement in synthesis context.

- Refactor MemoryInitSpec for improved results readability

Context:

PR #2166 (commit: 4530152) introduced MemorySynthInit annotation to control whether statement generated with Memory*InitAnnotation (emitted within initial begin block in verilog) should be guarded with ifndef SYNTHESIS or not.
Unfortunately only one configuration (MemoryFileInlineAnnotation) has been tested while the others have been generating incorrect verilog statements (MemoryArrayInitAnnotation and MemoryScalarInitAnnotation).

Signed-off-by: Jean Bruant <[email protected]>
(cherry picked from commit 475c165)

Co-authored-by: John's Brew <[email protected]>
mergify bot added a commit that referenced this pull request Jan 27, 2022
* Fix faulty MemorySynthInit behavior (#2468)

- Fix & test MemorySynthInit behavior with MemoryArrayInitAnnotation and MemoryScalarInitAnnotation.
Add test case for MemoryRandomInitAnnotation which is, on the contrary, expected not to leak any randomization statement in synthesis context.

- Refactor MemoryInitSpec for improved results readability

Context:

PR #2166 (commit: 4530152) introduced MemorySynthInit annotation to control whether statement generated with Memory*InitAnnotation (emitted within initial begin block in verilog) should be guarded with ifndef SYNTHESIS or not.
Unfortunately only one configuration (MemoryFileInlineAnnotation) has been tested while the others have been generating incorrect verilog statements (MemoryArrayInitAnnotation and MemoryScalarInitAnnotation).

Signed-off-by: Jean Bruant <[email protected]>
(cherry picked from commit 475c165)

# Conflicts:
#	src/main/scala/firrtl/backends/verilog/VerilogEmitter.scala

* Fix conflict

Co-authored-by: John's Brew <[email protected]>
Co-authored-by: Jean Bruant <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Backported This PR has been backported to marked stable branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

readmem block is not picked up by synthesis tools
3 participants