Support for verilog memory loading. #840

chick · 2018-06-26T00:14:01Z

Implements PR #835

Type of change: enhancement
Impact: API addition (no impact on existing code)
Development Phase: implementation

Release Notes

The new annotation ChiselLoadMemoryAnnotation provides support for a memory to be
initialized during simulation. This uses the verilog $readmemh or $readmemb to provide a
text file with numeric values that can be loaded into the memory at the beginning of simulation.

Usage

Simple Memory

import chisel3.util.loadMemoryFromFile
...
// within a module
  val memory = Mem(memoryDepth, UInt(32.W))
  loadMemoryFromFile(memory, "mem_data")  // actual file name must be mem_data.txt

The default input format is hex using $readmemh. Binary text can be used instead via

  loadMemoryFromFile(memory, "mem_data", hexOrBinary = "b")

Memory can be an aggregation

class MemoryShape extends Bundle {
  val a = UInt(8.W)
  val b = SInt(8.W)
  val c = Bool()
}

class HasComplexMemory(memoryDepth: Int) extends Module {
 ...
  val memory = Mem(memoryDepth, new MemoryShape)
  loadMemoryFromFile(memory, "mem_data")

This memory will be broken up by the Firrtl compiler into three separate memories
memory_a, memory_b, memory_c
There must be a separate load file for each of these memories with the names
mem_data_a.txt, mem_data_b.txt, mem_data_c.txt

These files are not checked at compile time. It might be a good idea to give them full path names.

Input file format.

Standard verilog file compatible with $readmemh or $readmemb. Data has to exist in a text file. White space is allowed to improve readability, as well as comments in both single line and block. The numbers have to be stored as binary or hexadecimal values. The basic form of a memory file contains numbers separated by new line characters that will be loaded into the memory.

Implementation details

Using these annotations will create parallel modules using Chisel3's in-line black boxes and these parallel modules will be connected to the unaltered modules using verilog's bind statement.

* first pass * create annotation * create skeleton Transform

Building out transform and pass now

* Creates chisel and firrtl LoadMemory annotations * LoadMemoryTransform converts annotation into BlackBox InLine * Simple test that verilog bound modules get created.

* Supports Bundled/multi-field memories * more tests * support for `$readmemh` and `$readmemb` * warns if suffix used in file specification.

* Use standard chisel annotation idiom

chick · 2018-06-26T00:27:04Z

This is a trial balloon for this RFC, I think I am pretty close to the functionality I hoped to support.
Still to come are hooks for treadle/interpreter.
Particular targets of scrutiny:

File naming conventions for the bindable modules, there could be collisions here
More checking of input files (this is tricky because of splitting bundled memories)
Error/warning handling.

seldridge

Broadly, this looks fine. I haven't gone through the companion FIRRTL PR, yet (I was very confused when I saw getRenderer...).

Three major points:

I'd suggest letting the user specify whatever filename they want. There's no standard file extension that I'm aware of for $readmemh/$readmemb (or is specified in the Verilog specifications as far as I can tell).
The instance walk I think is unnecessary here. The user can only annotate at the ComponentName level (annotating memories inside modules). That disallows disambiguation based on module instance where the memory lives. I think that this PR should only look for module name matches and call it a day.
I'd like to see more end-to-end tests in this PR to make sure that the memory loading is definitely working. That could be delegated off to the companion FIRRTL PR, though.

A bunch of minor nits.

seldridge · 2018-06-26T17:08:15Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+import chisel3.core.RunFirrtlTransform
+import chisel3.internal.{Builder, InstanceId}
+import firrtl.annotations._
+import firrtl.ir.{Module => _, _}


Nit: should this PR follow the style guide suggestion of explicit imports for anything not import chisel3._?

Nit: I've seen Jack using import firrtl.{ir => fir} which I kind of liked. Perhaps consider that and you can avoid the need to exclude Module and then use it's full package path below.

Which style guide are you referring to here?

Sorry, didn't respond... The one sitting in doc/style.md.

seldridge · 2018-06-26T17:11:22Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+  * @param target        memory to load
+  * @param fileName      name of input file
+  * @param hexOrBinary   use $readmemh or $readmemb
+  */case class ChiselLoadMemoryAnnotation(target: InstanceId, fileName: String, hexOrBinary: String = "h")


I'd suggest using scala.util.Enumeration for the hexOrBinary parameter. It makes it explicit what is actually allowed and avoids the need for the argument checking below.

seldridge · 2018-06-26T17:18:50Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+  * @param fileName      name of input file
+  * @param hexOrBinary   use $readmemh or $readmemb
+  */case class ChiselLoadMemoryAnnotation(target: InstanceId, fileName: String, hexOrBinary: String = "h")
+  extends chisel3.core.ChiselAnnotation


I think it would be clearer if this PR had all imports specified up top (unless there was some need for package name conflicts here).

seldridge · 2018-06-26T17:21:03Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+        s"""LoadMemoryAnnotation fileName "$fileName" has extension ".txt" will still be appended"""
+      )
+    case _ =>
+  }


The usage of .txt isn't canonical (as far as I can tell). The System Verilog standard (1800-2017) seems to indicate that the filename can be anything. This PR can avoid the necessary checking and append logic by just using whatever the user gives. This may avoid problems later on due to the diversity of examples online that a user may follow. The SV spec uses an example of mem.data, but online I've seen .hex, .list, etc. I think it's easier to trust the user (and it makes the eventual option parsing easier 😄).

My problem here is that when the memory is an aggregate it puts a heavier burden on figuring out what the naming convention should be. My naive hope was that by the code auto adding the suffix we could keep it simpler. if they control the suffix, what do you think I should do. Ask for a place holder like "memory_?.hex" where ? will be replaced by the memory subfield.

You're absolutely right. Additionally, there's zero reason to try and provide spec compliance. We're in control of everything here and this will be presented to the user, assumedly, as an API for loading memory, not an API for accessing readmemh/b Verilog primitives. We could have the user pass a function in or something, but that smells over-engineered. I'm good with the .txt and your strategy here. Thanks for the explanation.

seldridge · 2018-06-26T17:23:40Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+    case _ =>
+  }
+
+  def transformClass : Class[LoadMemoryTransform] = classOf[LoadMemoryTransform]


Nit: transformClass: Class

I'm not sure what you mean here

def transformClass: Class = classOf[LoadMemoryTransform]

does not compile.

I think he means that there should be no space between transformClass and the :

seldridge · 2018-06-26T17:49:20Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+      case Some(externalModule: DefModule) =>
+        externalModule
+      case _ =>
+        throw new Exception(s"Could not find module $moduleName in circuit $circuit")


Nit: there's no convention here, but maybe FIRRTLException?

Using the error system between firrtl and chisel has been a problem for me. Don't tell anyone but half the reason I implemented the composite memory logic was that I was having trouble figuring out how to generate errors when that occurred.
@jackkoenig what's the best way to do this. This transform is in chisel3 but is invoked in firrtl. All too often errors I generate create the particularly unhelpful message "File a Firrtl Issue" even though in my case it's a simple user error. Should I follow the model of something like CheckComboLoops or is there a better example. I looked around a little. This seems like it would be pretty useful wiki page

No reason for this to hold it up either. My (unofficial) strategy has been to have anything run by the tool throw that type of exception. It's entirely unnecessary, though. Yeah, no need for throwInternalError (we don't want new issues filed here...). Maybe a question for the group and to be added to the wiki as you suggest.

seldridge · 2018-06-26T18:37:01Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+
+    def makePath(componentName: String): String = {
+      circuitState.circuit.main + "." + myModule.name + "." + componentName
+    }


I think that this is equivalent to the serialize method of ComponentName.

It is, I'm going to leave this for now pending a more general discussion of what a ComponentName actually is.

seldridge · 2018-06-26T18:42:14Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+      } match {
+        case Some(lma @ LoadMemoryAnnotation(
+        ComponentName(componentName, moduleName: ModuleName), _, hexOrBinary, _
+        )) =>


Nit: I think you can do this on one line.

Is the explicit ModuleName type necessary?

Nope, cut and paste artifact

seldridge · 2018-06-26T19:08:42Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+          val subModule = findModule(moduleName, circuitState.circuit)
+          val newPrefix = (if (modulePrefix.nonEmpty) modulePrefix + "." else "") + myModule.name
+
+          processModule(newPrefix, subModule)


Without instance annotations, I'm not convinced that this PR needs to do an instance walk (and if it did, I'd expect to see use of InstanceGraph). The expressiveness of a LoadMemoryAnnotation is limited to components. I think this PR can just loop over all modules that have annotations targeting their components. An explicit instance walk would seem to visit multiple modules multiple times which, without instance annotations, should be unnecessary.

This brings up a larger problem of how a user can use this to initialize different memories differently, e.g., the boot ROMs of disparate Rocket cores. Likely out of scope without a good formalism for instance annotations.

seldridge · 2018-06-26T19:12:41Z

src/test/scala/chiselTests/LoadMemoryFromFileSpec.scala

+    output should include ("""1 warning(s)""")
+  }
+
+}


I'd like to see this PR add some end-to-end testing of memories. I.e., add some resources of readmemh/readmemb format and then make sure that the data matches up.

I have end to end testing in an as-yet-unpushed PR on chisel-testers because of all the heavy lifting that is done there to invoke verilator with all the right arguments and file names. Because of the testers2 project Richard is working on I'm not really sure where that is all going to live come 3.2, but I think it is as yet un-PR'd chisel3

Sounds great. I should have expected that you had something lined up...

@seldridge

* Fixes for @seldridge nits and super-nits

chick · 2018-06-26T23:51:27Z

New commit answers most nits. Remaining issues

What are best practices for error handling in external transformations
What to do about multiple instantiations of a Module that has a memory annotated for readmem
- Right now I think it will add a readmem for each one.
What to do about file naming, currently I insist that I will control the suffix as .txt
- Is some kind of naming template needed.
- Otherwise I like the way aggregate memories are handled

jackkoenig · 2018-06-27T02:06:04Z

What are best practices for error handling in external transformations

This is a great question. Currently we treat anything that is not a FIRRTLException (or a couple of other types) as something that should never happen and is therefore an internal error worth reporting on FIRRTL. That is definitely not the case so perhaps we should change FIRRTL to inspect the Exception stack trace to determine if it's actually a FIRRTL internal error and forward the exception otherwise?

What to do about multiple instantiations of a Module that has a memory annotated for readmem
Right now I think it will add a readmem for each one.

I think if you bind an ExtModule with the readmem to the Module it will apply to every instance of the Module with the memory. If you want support for giving different values to different instances of the same Module, using bind won't work in Verilator (but will in VCS).

What to do about file naming, currently I insist that I will control the suffix as .txt

I think that all of this is a symptom of FIRRTL's support for aggregate memories being weird. Rather than splitting them up, we could consider essentially casting aggregate types to UInt and packing and unpacking the aggregates at the ports. repl-seq-mems has all of the logic to do this, we essentially would just not blackbox the memory but instead leave it there with a UInt type. Our memory emission would need support for masks but other than that probably wouldn't need any modification.

jackkoenig · 2018-07-06T20:04:06Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+  */
+case class ChiselLoadMemoryAnnotation(
+  target     : InstanceId,
+  fileName   : String,


Rather than filename, I would call this prefix and also add a field for file extension since people may have their own preferred file extension for these kinds of files.

This is now the filename, if aggregate memories are involved I use the filename as a template and
insert the sub-element name before the suffix. So I think that makes filename the right name for the var.
There is wiki documentation that explains this to the user

- transform now only runs if emitter is an instance of VerilogEmitter - suffixes on memory text files are now respected - if suffix exists and memory is aggregate, aggregate sub-fields will now be inserted before suffix - every bind module created gets a unique number - this is required when multiple loaded memories appear in a module - this should be generalized for other uses of binding modules

- remove un-needed suffix test

- remove instance walk, now just processes each module

chick · 2018-07-19T06:32:01Z

@jackkoenig @seldridge
Fixed comments and nits
Now handles suffixes on specified file names
No longer does instance walk
Full functionality tests are in new Chisel-testers PR 206

chick · 2018-07-19T17:57:09Z

I have added a Chisel Memories to the wiki. Other memory description should go there too, comments or edits welcome

- Move LoadMemoryTransformation into Firrtl for treadle to access it.

- One more bug in suffix handling has been eliminated

chick · 2018-07-20T18:48:38Z

After another go around, this RFC is more readier than ever. I have added treadle support also and chisel testers has better example tests. The PR's are

Firrtl Support for load memory annotations in chisel #833
Treadle Support for loading memories from files at startup #53
Chisel3 Support for verilog memory loading. #840
Chisel-testers Test memory loading #206

chick · 2018-07-26T18:19:01Z

please retest

chick · 2018-08-17T21:34:04Z

retest this please

chick · 2018-08-21T19:36:27Z

Ping @jackkoenig @seldridge
Can you re-look at this at your convenience. Firrtl is now merged, tests pass

jackkoenig · 2018-08-22T20:26:53Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+  */
+//TODO: (chick) better integration with chisel end firrtl error systems
+//scalastyle:off method.length cyclomatic.complexity regex
+class CreateBindableMemoryLoaders(circuitState: CircuitState) extends Pass {


Any reason this extends Pass instead of Transform?

Not really, just attempting to follow what I thought was the pattern, LoadMemoryTransform subclasses Transform and uses CreateBindableMemoryLoaders as its pass. I'm happy to follow a different pattern if there is a better one.

I would just make it a Transform, existing Passes in FIRRTL are kind of a relic of the past. That would also make it clear that this must run on LowForm rather than that information being encoded in the the other Transform below. I mainly bring this up because at first glance it was not obvious to me why you were handling Block but not Conditionally in processStatements below.

jackkoenig

Excited to see this coming together, some comments, requests, food for thought, etc.

jackkoenig · 2018-08-22T20:33:27Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+          circuitState.circuit.modules
+            .filter { module => module.name == moduleName.name }
+            .collectFirst { case m: firrtl.ir.Module => m }
+            .foreach { module =>


Why not just:

circuitState.circuit.modules.collectFirst { case m: firrtl.ir.Module if m.name == moduleName.name => ... }

Actually, similar to the proposed lookup for the LoadMemoryAnnotations above, there should just be a lookup for Modules from their name, the moduleMap created below could be hoisted out and serve that purpose right?

jackkoenig · 2018-08-24T18:41:25Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+  * @param hexOrBinary   use $readmemh or $readmemb
+  */
+case class ChiselLoadMemoryAnnotation(
+  target:      InstanceId,


Is there any reason to make the target InstanceId or should it be the more precise MemBase as in loadMemoryFromFile.apply below?

I did not have any luck changing this. Could not get it to compile despite a number of attempts to work with MemBase type parameters

Here's a diff that accomplishes what I would like to see

diff --git a/src/main/scala/chisel3/util/LoadMemoryTransform.scala b/src/main/scala/chisel3/util/LoadMemoryTransform.scala index 40aeb007..480d0467 100644 --- a/src/main/scala/chisel3/util/LoadMemoryTransform.scala +++ b/src/main/scala/chisel3/util/LoadMemoryTransform.scala @@ -2,8 +2,8 @@ package chisel3.util -import chisel3.MemBase -import chisel3.core.{ChiselAnnotation, RunFirrtlTransform, annotate} +import chisel3._ +import chisel3.experimental.{ChiselAnnotation, RunFirrtlTransform, annotate} import chisel3.internal.{Builder, InstanceId} import firrtl.annotations.{MemoryLoadFileType, _} import firrtl.ir.{Module => _, _} @@ -18,8 +18,8 @@ import scala.collection.mutable * @param fileName name of input file * @param hexOrBinary use $readmemh or $readmemb */ -case class ChiselLoadMemoryAnnotation( - target: InstanceId, +case class ChiselLoadMemoryAnnotation[T <: Data]( + target: MemBase[T], fileName: String, hexOrBinary: MemoryLoadFileType.FileType = MemoryLoadFileType.Hex ) @@ -40,8 +40,8 @@ case class ChiselLoadMemoryAnnotation( object loadMemoryFromFile { - def apply( - memory: MemBase[_], + def apply[T <: Data]( + memory: MemBase[T], fileName: String, hexOrBinary: MemoryLoadFileType.FileType = MemoryLoadFileType.Hex ): Unit = {

jackkoenig · 2018-08-24T18:44:23Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+  */
+//TODO: (chick) better integration with chisel end firrtl error systems
+//scalastyle:off method.length cyclomatic.complexity regex
+class CreateBindableMemoryLoaders(circuitState: CircuitState) extends Pass {


I would just make it a Transform, existing Passes in FIRRTL are kind of a relic of the past. That would also make it clear that this must run on LowForm rather than that information being encoded in the the other Transform below. I mainly bring this up because at first glance it was not obvious to me why you were handling Block but not Conditionally in processStatements below.

jackkoenig · 2018-08-24T18:47:47Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+//TODO: (chick) better integration with chisel end firrtl error systems
+//scalastyle:off method.length cyclomatic.complexity regex
+class CreateBindableMemoryLoaders(circuitState: CircuitState) extends Pass {
+  var memoryCounter: Int = -1


Rather than a global var, I'd rather see a Namespace used for both bindsToName and the instance name below since it's technically possible we could have collisions, it could just be an argument to processModule

I'd prefer to not solve this problem right now. This transform should only run once so collisions should not be a problem. I think the name space problem for bind-able modules should be solved separately with more uses cases brought to bear. For the moment this variable is enough to prevent self collisions

Pass eliminated, LoadMemoryTransform does all the work itself now.

jackkoenig · 2018-08-24T18:48:18Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+
+  val bindModules:       mutable.ArrayBuffer[BlackBoxInlineAnno] = new mutable.ArrayBuffer()
+
+  val verilogEmitter:    VerilogEmitter = new VerilogEmitter


All of these should probably be private

moved some of them within run function
made bindModules private

jackkoenig · 2018-08-24T18:51:36Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+    def processMemory(name: String): Unit = {
+      val fullMemoryName = makePath(s"$name")
+
+      memoryAnnotations.find {


Rather than doing a linear search of all of the LoadMemoryAnnotations for every single memory (O(n*m) where n is number of memories and m is number of LoadMemoryAnnotations), we could instead create a nested lookup table, maybe Map[String, Map[String, LoadMemoryAnnotation]] where keys of the outer map are module names and inner map are memory instance names.

jackkoenig · 2018-08-24T18:54:09Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+            processStatements(subStatement)
+          }
+
+        case m: DefAnnotatedMemory => processMemory(m.name)


DefAnnotatedMemorys should not leak from ReplSeqMems, so I don't think they should be handled here. Also I think the newlines here are unnecessary but that's just a stylistic nit.

jackkoenig · 2018-08-24T18:57:55Z

src/test/scala/chiselTests/ComplexMemoryLoadingSpec.scala

+        }
+
+      case _=>
+        println(s"Failed compile")


Could you replace println(s"Failed compile") with fail(s"Failed compile") just to be sure this test fails if ChiselExecutionFailure is returned?

jackkoenig · 2018-08-24T18:58:06Z

src/test/scala/chiselTests/ComplexMemoryLoadingSpec.scala

+          val file = new File(dir, s"HasComplexMemory.HasComplexMemory.memory_$element.v")
+          file.exists() should be (true)
+          val fileText = io.Source.fromFile(file).getLines().mkString("\n")
+          fileText should include (s"""$$readmemh("./mem_$element", HasComplexMemory.memory_$element);""")


Nice test :)

jackkoenig · 2018-08-24T18:59:49Z

src/main/scala/chisel3/util/BindPrefixFactory.scala

+
+import scala.collection.mutable
+
+class BindPrefixFactory(basePrefix: String) {


This isn't used, is it supposed to be?

@jkoenig

Fixes based on @jkoenig review - remove unused BindPrefixFactory - Moved code from CreateBindableMemoryLoaders into to LoadMemoryTransfrom - Made map to find relevant memory annotations faster - Made map to find modules referenced by annotations faster - Made things private that should be private - DefAnnotatedMemorys are no longer referenced, shouldn't be found here. - println of error changed to failed

chick · 2018-08-30T21:05:36Z

@jackkoenig @seldridge I think I have answered all change requests, can we get this unstuck, thanks

seldridge

This moves towards adding a much-needed chisel3 API. Nice work, @chick.

Some requests in the review (only one super-nit!).

src/main/scala/chisel3/util/LoadMemoryTransform.scala

seldridge · 2018-08-30T20:59:18Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+  * to do that.
+  */
+//noinspection ScalaStyle
+class LoadMemoryTransform extends Transform {


I expected that this would be in the FIRRTL tree as opposed to Chisel. Not that big of a deal, but that seems more logical to me for things which are FIRRTL-proper as opposed to libraries. What do you think?

I'm kind of in the opposite camp. I am more interested in examples of how to write things that are more like external libraries that do not require changes to the underlying firrtl code. This was a bit of a fail in that it required changes to firrtl to support generating modules bound to other module, but this should lead to exposing bound modules as an API for library developers. So hopefully this is sufficient justification for where this is.

I see what you're saying. Nothing like that exists in the Chisel tree (as far as I'm aware). External libraries show it, but that's all.

I guess then there's the reverse question of why LoadMemoryAnnotation and MemoryLoadFileType aren't in Chisel?

I think this occurred to me as reading the code I was bouncing back and forth between the repos: a chisel3.util.experimental.ChiselLoadMemoryAnnotation generates a firrtl.LoadMemoryAnnotation which is processed by a chisel3.util.experimental.LoadMemoryTransform which interacts with the the firrtl.VerilogEmitter.

I am trying to write as much of this stuff as a library without changing firrtl.
It's true that for this project firrtl did get changed but that was because we needed some low level support for access to verilog module headers to match signatures for module binding, but I think that's justifiable because it is the beginning of an API for other work with verilog binding.
Bottom line, I'd prefer things to stay where they are now.

Got it. Yeah, not the low level stuff. I'm only narrowly talking about the contents of FIRRTL's LoadMemoryAnnotation.scala. Moving that (and only that) over here would make this entirely encapsulated in Chisel. Maybe a subsequent PR? (Or, I can slam a PR through right now if that's something you want in this PR). No reason to hold this up for that, though.

seldridge · 2018-08-30T21:09:11Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+    Builder.warning(
+      s"""LoadMemory from file annotations file empty file name"""
+    )
+  }


This is supposed to be an error if it gets to post-elaboration toFirrtl (LoadMemoryAnnotation is supposed to be treating this as an exception). I'd suggest either not emitting a warning or emitting an error. I'm tempted to go with the former as I don't think there's significant time savings in failing during elaboration vs. post elaboration. @jackkoenig may be able to comment on the utility of this type of elaboration failure for large designs.

Note: the supposed to be qualifier above is due to me looking at how LoadMemoryAnnotation works. However, an empty filename isn't resulting in Nil as I think it's supposed to. See:

scala> "".split("""\.""").toList == Nil res15: Boolean = false

This should probably just be:

require(fileName.nonEmpty, "LoadMemory from file annotations file empty file name")

I took the easy way out here and Changed to Builder.error. I don't think it hurts to do it sooner. I don't think it's a particularly likely case anyway.

I made it an exception, mostly to get rid of the Builder reference.
This is a relatively unlikely (and clearly wrong) edge case. I'd rather keep the fixes simple as possible.

src/main/scala/chisel3/util/LoadMemoryTransform.scala

src/test/scala/chiselTests/LoadMemoryFromFileSpec.scala

src/test/scala/chiselTests/ComplexMemoryLoadingSpec.scala

jackkoenig · 2018-08-31T00:04:28Z

src/main/scala/chisel3/util/LoadMemoryTransform.scala

+
+import chisel3.MemBase
+import chisel3.core.{ChiselAnnotation, RunFirrtlTransform, annotate}
+import chisel3.internal.{Builder, InstanceId}


Things in package chisel3.util should try to be like what any library would look like so it should not reach into chisel3.core (import stuff directly from chisel3_ and chisel3.experimental._ instead).

Also it should avoid use of chisel3.internal, and avoid private[chisel3] stuff (like the Builder). I think the only use of Builder is for a warning below that should probably just be an exception, or better yet, a requirement below.

Done, I had to add experimental.annotate and chisel3.InstanceId to the chisel package. But I think that is correct as these are part of the public API

chick · 2018-08-31T04:27:19Z

@jackkoenig @seldridge : I have made the suggested changes in nearly all instance. Those that were straight forward coding I marked the sub-issue as resolved. There's a couple of responses not marked resolved, but I think I am close enough that they can be considered resolved for now.
Please review again (thanks) because I would love to get this set of PRs out of my working memory :-)

One other small note, I moved the ComplexTest into the LoadMemoryFromFileSpec because I think it is nicer having both cases tested in the same place.

- Many changes based on review - move stuff into experimental - clean up annotation manipulation - manage tests better - use more standard practices for transform

seldridge

lgtm

Nits related to stray comments and a thought about moving the body of the ChiselLoadMemoryAnnotation ScalaDoc to the loadMemoryFromFile object.

Thanks for taking the time to write the (fantastic) ScalaDoc. Chisel Wiki and Chisel Testers pointers makes a ton of sense, too. This is a pain, but I think it pays dividends for a user-facing API. 👍

Also, no offense taken if you include the Fixes for @seldridge nits and super-nits in the commit log.

seldridge · 2018-08-31T05:02:24Z

src/main/scala/chisel3/package.scala

@@ -245,6 +245,8 @@ package object chisel3 {    // scalastyle:ignore package.object.name

  type BlackBox = chisel3.core.BlackBox

+  type InstanceId = chisel3.internal.InstanceId


This is a question for @jackkoenig if this should be exposed outside of internal. I've had to use this for external libraries, so it likely makes sense.

I think InstanceId is the wrong API and I wish it weren't necessary to expose it but it's obviously the only API so what can we do.

That being said, I think this PR should not be using InstanceId, see my comment below on the ChiselAnnotation

Got it. Thanks for figuring out a way around this.

Since the PR no longer needs InstanceId, should we still be doing this or have a separate PR more focused on that question?

seldridge · 2018-08-31T05:04:50Z

src/main/scala/chisel3/util/experimental/LoadMemoryTransform.scala

+  * the relevant module by using the creating separate modules that are bound to the modules containing
+  * the memories to be loaded.
+  *
+  * ==Example module==


Great documentation. I'd move the body onto the loadMemoryFromFile object.

Moved doc code to loadMemoryFromFile which is how annotation is created

seldridge · 2018-08-31T05:05:46Z

src/test/scala/chiselTests/LoadMemoryFromFileSpec.scala

+import firrtl.annotations.MemoryLoadFileType
+import org.scalatest.{FreeSpec, Matchers}
+
+//noinspection TypeAnnotation


I prefer to prevent IntelliJ from getting to commit to the repo... Is it okay to remove this?

I guess I'm in the other camp, where I really like to have my files "style clean", with annotations like this for when I intentionally violate the rules. I have changed this to
//scalastyle:off method.length
Which is more specific and sufficient to make the file get a ✅
I hope that's ok.

Scalastyle comments lgtm.

seldridge · 2018-08-31T05:05:52Z

src/test/scala/chiselTests/LoadMemoryFromFileSpec.scala

+  val low2 = Module(new UsesMemLow(memoryDepth, memoryType))
+
+//  doNotDedup(low1)
+//  doNotDedup(low2)


seldridge · 2018-08-31T05:06:52Z

src/test/scala/chiselTests/LoadMemoryFromFileSpec.scala

+  io.value := memory(io.address)
+}
+
+//noinspection TypeAnnotation


src/test/scala/chiselTests/LoadMemoryFromFileSpec.scala

jackkoenig · 2018-08-31T17:36:28Z

src/main/scala/chisel3/util/experimental/LoadMemoryTransform.scala

+  target:      InstanceId,
+  fileName:    String,
+  hexOrBinary: MemoryLoadFileType.FileType = MemoryLoadFileType.Hex
+)


I think it got lost in Github's aggressive "hide outdated conversations 'feature'", but here is a diff that makes this more type safe and removes the need for InstanceId:

diff --git a/src/main/scala/chisel3/util/experimental/LoadMemoryTransform.scala b/src/main/scala/chisel3/util/experimental/LoadMemoryTransform.scala index a81129d4..49a00637 100644 --- a/src/main/scala/chisel3/util/experimental/LoadMemoryTransform.scala +++ b/src/main/scala/chisel3/util/experimental/LoadMemoryTransform.scala @@ -4,7 +4,6 @@ package chisel3.util.experimental import chisel3._ import chisel3.experimental.annotate -import chisel3.InstanceId import chisel3.experimental.{ChiselAnnotation, RunFirrtlTransform} import firrtl.annotations.{MemoryLoadFileType, _} import firrtl.ir.{Module => _, _} @@ -73,8 +72,8 @@ import scala.collection.mutable * @param fileName name of input file * @param hexOrBinary use $readmemh or $readmemb, i.e. hex or binary text input, default is hex */ -case class ChiselLoadMemoryAnnotation( - target: InstanceId, +case class ChiselLoadMemoryAnnotation[T <: Data]( + target: MemBase[T], fileName: String, hexOrBinary: MemoryLoadFileType.FileType = MemoryLoadFileType.Hex ) @@ -95,8 +94,8 @@ case class ChiselLoadMemoryAnnotation( object loadMemoryFromFile { - def apply( - memory: MemBase[_], + def apply[T <: Data]( + memory: MemBase[T], fileName: String, hexOrBinary: MemoryLoadFileType.FileType = MemoryLoadFileType.Hex ): Unit = {

Note this diff is slightly different than the previous one, I updated it with the recent changes so that it would apply cleanly

jackkoenig · 2018-08-31T18:34:04Z

src/main/scala/chisel3/util/experimental/LoadMemoryTransform.scala

+      }
+
+    val modulesByName = {
+      circuit.modules.collect { case m: firrtl.ir.Module => m }.map { module => module.name -> module }.toMap


I still think this is better as just:

circuit.modules.collect { case m: firrtl.ir.Module => m.name -> m }.toMap

- More review changes - Move doc from annotation to the object apply method that generates the annotation - Make scalastyle directives more specific - Use more efficient collect to generate name to module map - Made lines obey style length limit - a couple of cleanups of imports in tests - removed some commented out code - optimized checking for lines using .exists - use _ for unused variable in match

chick · 2018-08-31T18:57:13Z

@jackkoenig @seldridge Pushed latest changes. Hoping for approval soon, thanks

@seldridge

* Ability to load memories at simulation startup * first pass * create annotation * create skeleton Transform * Work in progress Building out transform and pass now * Support for LoadMemory annotation * Creates chisel and firrtl LoadMemory annotations * LoadMemoryTransform converts annotation into BlackBox InLine * Simple test that verilog bound modules get created. * Support for LoadMemory annotation * Supports Bundled/multi-field memories * more tests * support for `$readmemh` and `$readmemb` * warns if suffix used in file specification. * Support for LoadMemory annotation * Use standard chisel annotation idiom * Support for LoadMemory annotation * Fixes for @seldridge nits and super-nits * Support for LoadMemory annotation - transform now only runs if emitter is an instance of VerilogEmitter - suffixes on memory text files are now respected - if suffix exists and memory is aggregate, aggregate sub-fields will now be inserted before suffix - every bind module created gets a unique number - this is required when multiple loaded memories appear in a module - this should be generalized for other uses of binding modules * Support for LoadMemory annotation - remove un-needed suffix test * Support for LoadMemory annotation - remove instance walk, now just processes each module * Support for LoadMemory annotation - Move LoadMemoryTransformation into Firrtl for treadle to access it. * Support for LoadMemory annotation - One more bug in suffix handling has been eliminated * Support for LoadMemory annotation - remove unused findModule per jackkoenig - fixed complex test, bad filename edge case * Support for LoadMemory annotation - changed to not use intellij style column alignment for : declarations * Load memory from file Fixes based on @jkoenig review - remove unused BindPrefixFactory - Moved code from CreateBindableMemoryLoaders into to LoadMemoryTransfrom - Made map to find relevant memory annotations faster - Made map to find modules referenced by annotations faster - Made things private that should be private - DefAnnotatedMemorys are no longer referenced, shouldn't be found here. - println of error changed to failed * Loading memories from files - Many changes based on review - move stuff into experimental - clean up annotation manipulation - manage tests better - use more standard practices for transform * Loading memories from files - More review changes - Move doc from annotation to the object apply method that generates the annotation - Make scalastyle directives more specific - Use more efficient collect to generate name to module map - Made lines obey style length limit - a couple of cleanups of imports in tests - removed some commented out code - optimized checking for lines using .exists - use _ for unused variable in match (cherry picked from commit a5185d2)

chick added 5 commits June 11, 2018 17:24

Ability to load memories at simulation startup

521d77e

* first pass * create annotation * create skeleton Transform

Work in progress

76a9b91

Building out transform and pass now

Support for LoadMemory annotation

118bb7d

* Creates chisel and firrtl LoadMemory annotations * LoadMemoryTransform converts annotation into BlackBox InLine * Simple test that verilog bound modules get created.

Support for LoadMemory annotation

6cc1353

* Supports Bundled/multi-field memories * more tests * support for `$readmemh` and `$readmemb` * warns if suffix used in file specification.

Support for LoadMemory annotation

0e5cc30

* Use standard chisel annotation idiom

chick self-assigned this Jun 26, 2018

chick requested review from seldridge and jackkoenig June 26, 2018 00:14

chick added the Feature New feature, will be included in release notes label Jun 26, 2018

chick added this to the 3.2.0 milestone Jun 26, 2018

seldridge requested changes Jun 26, 2018

View reviewed changes

Support for LoadMemory annotation

4f55af1

* Fixes for @seldridge nits and super-nits

Merge branch 'master' into load-mem

14cea70

jackkoenig reviewed Jul 6, 2018

View reviewed changes

azidar requested review from azidar and removed request for azidar July 13, 2018 20:09

chick added 3 commits July 18, 2018 22:52

Support for LoadMemory annotation

24571c7

- remove un-needed suffix test

Support for LoadMemory annotation

8a3bd21

- remove instance walk, now just processes each module

chick added 2 commits July 19, 2018 22:32

Support for LoadMemory annotation

8df88d9

- Move LoadMemoryTransformation into Firrtl for treadle to access it.

Support for LoadMemory annotation

66fc4ca

- One more bug in suffix handling has been eliminated

Merge branch 'master' into load-mem

88bf995

jackkoenig reviewed Aug 22, 2018

View reviewed changes

Merge branch 'master' into load-mem

cbf1d49

jackkoenig requested changes Aug 24, 2018

View reviewed changes

chick added 2 commits August 24, 2018 14:41

Merge branch 'master' into load-mem

a6d8ee5

seldridge self-requested a review August 30, 2018 19:24

Merge branch 'master' into load-mem

38a1343

seldridge requested changes Aug 30, 2018

View reviewed changes

jackkoenig reviewed Aug 31, 2018

View reviewed changes

chick added 2 commits August 30, 2018 21:31

Loading memories from files

60e9627

- Many changes based on review - move stuff into experimental - clean up annotation manipulation - manage tests better - use more standard practices for transform

Merge remote-tracking branch 'origin/load-mem' into load-mem

ffb8d2b

seldridge approved these changes Aug 31, 2018

View reviewed changes

jackkoenig reviewed Aug 31, 2018

View reviewed changes

jackkoenig approved these changes Aug 31, 2018

View reviewed changes

chick merged commit a5185d2 into master Aug 31, 2018

wsong83 mentioned this pull request Sep 3, 2018

bi-weekly report 2018-09-16 cnrv/home#117

Merged

6 tasks

chick mentioned this pull request Nov 21, 2018

Build API for memory initialization from file. #639

Closed

ucbjrl modified the milestones: 3.2.0, 3.1.4 Dec 4, 2018

seldridge mentioned this pull request Feb 6, 2019

Emit fewer _T variables #1010

Closed

edwardcwang deleted the load-mem branch March 15, 2019 20:08

albert-magyar mentioned this pull request Mar 2, 2020

SyncReadMem.read(x) and Mem.read(RegNext(x)) behave differently #1366

Closed


		val bindModules: mutable.ArrayBuffer[BlackBoxInlineAnno] = new mutable.ArrayBuffer()

		val verilogEmitter: VerilogEmitter = new VerilogEmitter


		import scala.collection.mutable

		class BindPrefixFactory(basePrefix: String) {

		@@ -245,6 +245,8 @@ package object chisel3 { // scalastyle:ignore package.object.name

		type BlackBox = chisel3.core.BlackBox

		type InstanceId = chisel3.internal.InstanceId

Support for verilog memory loading. #840

Support for verilog memory loading. #840

Conversation

chick commented Jun 26, 2018 • edited Loading

Implements PR #835

Usage

Simple Memory

Memory can be an aggregation

Input file format.

Implementation details

chick commented Jun 26, 2018

seldridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chick commented Jun 26, 2018 • edited Loading

jackkoenig commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chick commented Jul 19, 2018

chick commented Jul 19, 2018

chick commented Jul 20, 2018

chick commented Jul 26, 2018

chick commented Aug 17, 2018

chick commented Aug 21, 2018

Choose a reason for hiding this comment

chick Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackkoenig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chick commented Aug 30, 2018

seldridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chick commented Jun 26, 2018 •

edited

Loading

chick commented Jun 26, 2018 •

edited

Loading

chick Aug 22, 2018 •

edited

Loading

chick Aug 31, 2018 •

edited

Loading