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

Support for verilog memory loading. #840

Merged
merged 24 commits into from Aug 31, 2018

Conversation

Projects
None yet
5 participants
@chick
Copy link
Contributor

commented Jun 26, 2018

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.

chick added some commits Jun 12, 2018

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

@chick chick self-assigned this Jun 26, 2018

@chick chick requested review from seldridge and jackkoenig Jun 26, 2018

@chick chick added the API Addition label Jun 26, 2018

@chick chick added this to the 3.2.0 milestone Jun 26, 2018

@chick

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2018

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
Copy link
Member

left a comment

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.

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

This comment has been minimized.

Copy link
@seldridge

seldridge Jun 26, 2018

Member

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

This comment has been minimized.

Copy link
@seldridge

seldridge Jun 26, 2018

Member

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.

This comment has been minimized.

Copy link
@chick

chick Jun 26, 2018

Author Contributor

Which style guide are you referring to here?

This comment has been minimized.

Copy link
@seldridge

seldridge Aug 30, 2018

Member

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

* @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")

This comment has been minimized.

Copy link
@seldridge

seldridge Jun 26, 2018

Member

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.

This comment has been minimized.

Copy link
@chick

chick Jun 26, 2018

Author Contributor

Ok

* @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

This comment has been minimized.

Copy link
@seldridge

seldridge Jun 26, 2018

Member

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).

This comment has been minimized.

Copy link
@chick

chick Jun 26, 2018

Author Contributor

Ok

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

This comment has been minimized.

Copy link
@seldridge

seldridge Jun 26, 2018

Member

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 😄).

This comment has been minimized.

Copy link
@chick

chick Jun 26, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@seldridge

seldridge Jun 27, 2018

Member

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.

case _ =>
}

def transformClass : Class[LoadMemoryTransform] = classOf[LoadMemoryTransform]

This comment has been minimized.

Copy link
@seldridge

seldridge Jun 26, 2018

Member

Nit: transformClass: Class

This comment has been minimized.

Copy link
@chick

chick Jun 26, 2018

Author Contributor

I'm not sure what you mean here

def transformClass: Class = classOf[LoadMemoryTransform]

does not compile.

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Jun 26, 2018

Contributor

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

This comment has been minimized.

Copy link
@chick

chick Jun 26, 2018

Author Contributor

Ahhh!

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

This comment has been minimized.

Copy link
@seldridge

seldridge Jun 26, 2018

Member

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

This comment has been minimized.

Copy link
@chick

chick Jun 26, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@seldridge

seldridge Jun 27, 2018

Member

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.


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

This comment has been minimized.

Copy link
@seldridge

seldridge Jun 26, 2018

Member

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

This comment has been minimized.

Copy link
@chick

chick Jun 26, 2018

Author Contributor

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

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

This comment has been minimized.

Copy link
@seldridge

seldridge Jun 26, 2018

Member
  • Nit: I think you can do this on one line.
  • Is the explicit ModuleName type necessary?

This comment has been minimized.

Copy link
@chick

chick Jun 26, 2018

Author Contributor

Nope, cut and paste artifact

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

processModule(newPrefix, subModule)

This comment has been minimized.

Copy link
@seldridge

seldridge Jun 26, 2018

Member

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.

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

}

This comment has been minimized.

Copy link
@seldridge

seldridge Jun 26, 2018

Member

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.

This comment has been minimized.

Copy link
@chick

chick Jun 26, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@seldridge

seldridge Jun 27, 2018

Member

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

Support for LoadMemory annotation
* Fixes for @seldridge nits and super-nits
@chick

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Jun 27, 2018

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.

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

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Jul 6, 2018

Contributor

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 comment has been minimized.

Copy link
@chick

chick Jul 26, 2018

Author Contributor

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

@azidar azidar requested review from azidar and removed request for azidar Jul 13, 2018

chick added some commits Jul 19, 2018

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
@chick

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2018

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

chick added some commits Jul 20, 2018

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
@chick

This comment has been minimized.

Copy link
Contributor Author

commented Jul 20, 2018

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

@chick

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2018

please retest

@chick

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2018

retest this please

@chick

This comment has been minimized.

Copy link
Contributor Author

commented Aug 21, 2018

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

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

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Aug 22, 2018

Contributor

Any reason this extends Pass instead of Transform?

This comment has been minimized.

Copy link
@chick

chick Aug 22, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Aug 24, 2018

Contributor

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
Copy link
Contributor

left a comment

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

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

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Aug 24, 2018

Contributor

Why not just:

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

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Aug 24, 2018

Contributor

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?

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

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Aug 24, 2018

Contributor

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

This comment has been minimized.

Copy link
@chick

chick Aug 24, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Aug 31, 2018

Contributor

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 = {
*/
//TODO: (chick) better integration with chisel end firrtl error systems
//scalastyle:off method.length cyclomatic.complexity regex
class CreateBindableMemoryLoaders(circuitState: CircuitState) extends Pass {

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Aug 24, 2018

Contributor

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.

//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

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Aug 24, 2018

Contributor

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

This comment has been minimized.

Copy link
@chick

chick Aug 24, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@chick

chick Aug 24, 2018

Author Contributor

Pass eliminated, LoadMemoryTransform does all the work itself now.


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

val verilogEmitter: VerilogEmitter = new VerilogEmitter

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Aug 24, 2018

Contributor

All of these should probably be private

This comment has been minimized.

Copy link
@chick

chick Aug 24, 2018

Author Contributor

moved some of them within run function
made bindModules private

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

memoryAnnotations.find {

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Aug 24, 2018

Contributor

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.

This comment has been minimized.

Copy link
@chick

chick Aug 24, 2018

Author Contributor

done

processStatements(subStatement)
}

case m: DefAnnotatedMemory => processMemory(m.name)

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Aug 24, 2018

Contributor

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.

This comment has been minimized.

Copy link
@chick

chick Aug 24, 2018

Author Contributor

done

}

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

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Aug 24, 2018

Contributor

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

This comment has been minimized.

Copy link
@chick

chick Aug 24, 2018

Author Contributor

done

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);""")

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Aug 24, 2018

Contributor

Nice test :)

This comment has been minimized.

Copy link
@chick

chick Aug 24, 2018

Author Contributor

thanks


import scala.collection.mutable

class BindPrefixFactory(basePrefix: String) {

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Aug 24, 2018

Contributor

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

This comment has been minimized.

Copy link
@chick

chick Aug 24, 2018

Author Contributor

gone

chick added some commits Aug 24, 2018

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

@seldridge seldridge self-requested a review Aug 30, 2018

@chick

This comment has been minimized.

Copy link
Contributor Author

commented Aug 30, 2018

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

@seldridge
Copy link
Member

left a comment

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

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

Show resolved Hide resolved src/main/scala/chisel3/util/LoadMemoryTransform.scala Outdated
Show resolved Hide resolved src/main/scala/chisel3/util/LoadMemoryTransform.scala Outdated
* to do that.
*/
//noinspection ScalaStyle
class LoadMemoryTransform extends Transform {

This comment has been minimized.

Copy link
@seldridge

seldridge Aug 30, 2018

Member

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?

This comment has been minimized.

Copy link
@chick

chick Aug 31, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@seldridge

seldridge Aug 31, 2018

Member

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.

This comment has been minimized.

Copy link
@chick

chick Aug 31, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@seldridge

seldridge Aug 31, 2018

Member

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.

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

This comment has been minimized.

Copy link
@seldridge

seldridge Aug 30, 2018

Member

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 comment has been minimized.

Copy link
@jackkoenig

jackkoenig Aug 31, 2018

Contributor

This should probably just be:

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

This comment has been minimized.

Copy link
@chick

chick Aug 31, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@chick

chick Aug 31, 2018

Author Contributor

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.

Show resolved Hide resolved src/main/scala/chisel3/util/LoadMemoryTransform.scala Outdated
Show resolved Hide resolved src/main/scala/chisel3/util/LoadMemoryTransform.scala Outdated
Show resolved Hide resolved src/main/scala/chisel3/util/LoadMemoryTransform.scala Outdated
Show resolved Hide resolved src/main/scala/chisel3/util/LoadMemoryTransform.scala Outdated
Show resolved Hide resolved src/test/scala/chiselTests/LoadMemoryFromFileSpec.scala Outdated
Show resolved Hide resolved src/test/scala/chiselTests/ComplexMemoryLoadingSpec.scala Outdated

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

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Aug 31, 2018

Contributor

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.

This comment has been minimized.

Copy link
@chick

chick Aug 31, 2018

Author Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2018

@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.

chick added some commits Aug 31, 2018

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
@seldridge
Copy link
Member

left a comment

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.

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

type BlackBox = chisel3.core.BlackBox

type InstanceId = chisel3.internal.InstanceId

This comment has been minimized.

Copy link
@seldridge

seldridge Aug 31, 2018

Member

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.

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Aug 31, 2018

Contributor

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

This comment has been minimized.

Copy link
@seldridge

seldridge Aug 31, 2018

Member

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

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Aug 31, 2018

Contributor

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

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

This comment has been minimized.

Copy link
@seldridge

seldridge Aug 31, 2018

Member

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

This comment has been minimized.

Copy link
@chick

chick Aug 31, 2018

Author Contributor

Moved doc code to loadMemoryFromFile which is how annotation is created

import firrtl.annotations.MemoryLoadFileType
import org.scalatest.{FreeSpec, Matchers}

//noinspection TypeAnnotation

This comment has been minimized.

Copy link
@seldridge

seldridge Aug 31, 2018

Member

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

This comment has been minimized.

Copy link
@chick

chick Aug 31, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@seldridge

seldridge Aug 31, 2018

Member

Scalastyle comments lgtm.

val low2 = Module(new UsesMemLow(memoryDepth, memoryType))

// doNotDedup(low1)
// doNotDedup(low2)

This comment has been minimized.

Copy link
@seldridge

seldridge Aug 31, 2018

Member

Remove.

io.value := memory(io.address)
}

//noinspection TypeAnnotation

This comment has been minimized.

Copy link
@seldridge

seldridge Aug 31, 2018

Member

Remove?

This comment has been minimized.

Copy link
@chick

chick Aug 31, 2018

Author Contributor

done

Show resolved Hide resolved src/test/scala/chiselTests/LoadMemoryFromFileSpec.scala
target: InstanceId,
fileName: String,
hexOrBinary: MemoryLoadFileType.FileType = MemoryLoadFileType.Hex
)

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Aug 31, 2018

Contributor

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

This comment has been minimized.

Copy link
@chick

chick Aug 31, 2018

Author Contributor

done

}

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

This comment has been minimized.

Copy link
@jackkoenig

jackkoenig Aug 31, 2018

Contributor

I still think this is better as just:

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

This comment has been minimized.

Copy link
@chick

chick Aug 31, 2018

Author Contributor

done

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
@chick

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2018

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

@chick chick merged commit a5185d2 into master Aug 31, 2018

4 checks passed

1 - compile
Details
2 - checkstyle
Details
2 - test
Details
pull request checks Build finished.
Details

@wsong83 wsong83 referenced this pull request Sep 3, 2018

Merged

bi-weekly report 2018-09-16 #117

6 of 6 tasks complete

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

ucbjrl added a commit that referenced this pull request Dec 13, 2018

Support for verilog memory loading. (#840)
* 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)

@edwardcwang edwardcwang deleted the load-mem branch Mar 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.