Direct to firrtl #829

jackkoenig · 2018-06-07T02:01:38Z

I think this still needs a little work, in particular the function converting Commands to Statements is a bit of a beast. I would appreciate suggestions on how to improve it! I'll provide some benchmarking numbers soon so that we can ensure this doesn't hurt performance (although for larger designs, skipping the parser should only help).

I should note that the "Converter" as currently implemented is kind of a transliteration of the existing Emitter. I would greatly appreciate thoughts on improvements. It is slightly more verbose than the Emitter because some emission logic lives within the Chisel IR and at least initially I decided to keep all conversion logic contained.

Type of change: other enhancement

Impact: API addition (no impact on existing code) -- You can now get FIRRTL directly in Chisel!

Development Phase: implementation

Release Notes
Directly translate from ChiselIR to FIRRTL without using the FIRRTL ANTLR Parser

This works around an assumption in FIRRTL that Literals never have UnknownWidth

chick · 2018-06-07T17:31:48Z

src/main/scala/chisel3/internal/firrtl/Converter.scala

+  // TODO refactor
+  def convert(cmds: Seq[Command], ctx: Component): fir.Statement = {
+    @tailrec
+    def rec(acc: Queue[fir.Statement],


What does rec mean?

You've probably considered this but could you create a trait like HasToStatement that most of the Commands could extend. Then the convert of command to statement could handle most of the cases with

case m: HasToStatement => rec(acc :+ m.toStatement, scope)(cmds.tail)

It changes the encapsulation of information, since for example, DefWire has to know how to make a fir.DefWire but it sure cleans up the massive match. I can see it's a bit ugly though since toStatement would need access to the Converter object for info, clocks etc.

rec is the name I give to inner recursive functions. It's arbitrary, I'm open to whatever. Scalaz uses go I think.

Yeah I have thought about it a bit but your point is well taken and I ought to share some of my thinking.

In prototyping this, I wanted to touch as little code as possible which is why the Converter is wholly self-contained, I am open to some amount of encapsulation though. toStatement would have to take the Component as an argument, but that's how def fullName currently works for converting Args to Expressions.

A big question related to performance I have that goes back to serialize in FIRRTL is whether large match statements or inheritance are better for performance. Serialization functions in this case are almost certainly megamorphic which is generally terrible for performance, but when we moved serialize from a match statement to a method on FIRRTLNode, it still got faster. Perhaps the dynamic method lookup in megamorphic state is still faster in practice than large match blocks. I'm really not sure.

jackkoenig · 2018-06-07T18:20:52Z

Currently the performance of this is terrible because the Seq of Commands is an ArrayBuffer, so repeatedly calling .tail on it is super bad. Simple solution is to convert to List before recursing, considering other options as well.

ducky64

Is it possible to have Chisel directly generate FIRRTL IR nodes, instead of going through chisel-firrtl? (was this a limitation of the different when structures, and can that be addressed with a restructuring of either Chisel or FIRRTL)?

I'm happy with the structure of this as a minimal-effort fix, but I'd prefer to avoid hax and clean things up properly if possible. Because I'd imagine that eventually we would merge the two IRs, might as well do it right the first time?

I haven't closely examined this for correctness.

Can you also quantify the performance gains - I'd guess the interesting metrics are runtime and memory consumption?

ducky64 · 2018-06-07T18:37:54Z

src/main/scala/chisel3/internal/firrtl/Converter.scala

+
+  // Whens markers are flat so scope must be inferred
+  // TODO refactor
+  def convert(cmds: Seq[Command], ctx: Component): fir.Statement = {


There's quite a bit of code here, can you write a summary of the algorithm in comments?
This mostly seems to be a straightforward 1:1 map of Chisel to FIRRTL types except with some funny stuff for whens.

Yeah this definitely needs a summary, will add

ducky64 · 2018-06-07T18:41:55Z

src/main/scala/chisel3/internal/firrtl/Converter.scala

+          val con = fir.Connect(convert(info), convert(loc, ctx), convert(exp, ctx))
+          rec(acc :+ con, scope)(cmds.tail)
+        case BulkConnect(info, loc, exp) =>
+          val con = fir.PartialConnect(convert(info), convert(loc, ctx), convert(exp, ctx))


Are FIRRTL PartialConnect semantics equivalent to Chisel BulkConnect semantics? Or should this exception out because this should never be seen?

FIRRTL PartialConnect semantics are closer to Chisel2 (and the current Chisel._ <>) semantics actually. It's still used by the compatibility wrapper.

seldridge

I'm in favor of this. It seems a lot cleaner to convert to a FIRRTL circuit and then (assumedly optionally) convert to a serialized String representation.

Are there any implications as it relates to CHIRRTL removal, i.e., chipsalliance/firrtl#727?

I'm also curious on the performance numbers...

seldridge · 2018-06-14T18:49:55Z

src/main/scala/chisel3/internal/firrtl/Converter.scala

+      val lit = bp.asInstanceOf[KnownBinaryPoint].value
+      fir.DoPrim(firrtl.PrimOps.AsFixedPoint, Seq(uint), Seq(lit), fir.UnknownType)
+    case lit: ILit => throwException(s"Internal Error! Unexpected ILit: $lit")
+  }


Stylistic nits. Per the style guide, you've got 120 characters to play with. For readability, you may consider putting some of these on one line. Additionally, I have a very hard time parsing dense match structures. You may consider doing an alignment on =>. Same for other large match blocks.

Yeah I agree the density is pretty tough. I ended up deciding to have all of the cases be multi-line. The matches were so different in length that alignment on => wasn't working very well

seldridge · 2018-06-14T19:25:25Z

src/main/scala/chisel3/internal/firrtl/Converter.scala

+  def convert(bp: BinaryPoint): fir.Width = bp match {
+    case UnknownBinaryPoint => fir.UnknownWidth
+    case KnownBinaryPoint(value) => fir.IntWidth(value)
+  }


I was confused on the exact replication of convert(BinaryPoint) and convert(Width), but this is how it's done in chisel3.internal.firrtl (with associated replication there). That may benefit from a refactor, but that is clearly out of scope of this encapsulated PR.

Yeah that was my thinking, a later refactor can clean a lot of stuff related to this and the Emitter

jackkoenig · 2018-06-14T22:39:31Z

I guess I should write down some of my thinking on this and let y'all know where I'm going with this:

It appears that the current ANTLR Parser for FIRRTL is the main culprit in the high memory use for FIRRTL. There are two (obvious) possible solutions:

Reduce the parser memory use
Don't use the parser

I spent some time trying to improve the parser (see chipsalliance/firrtl#824), but due to past experience and pain with the parser, I've been working on number 2. For this, I saw two main ways to do it:

Convert from Chisel to FIRRTL in memory and invoke FIRRTL in the same JVM
Use ProtoBuf (or other library) for serialization and deserialization

I originally was working on number 1 (see this PR), but it turns out to be a pretty complex game of wack-a-mole with static references to Chisel objects (in Chisel projects like rocket-chip) that prevent garbage collection (harming memory use substantially). To illustrate, using a large design, I get the following memory stats:

Elaborated Chisel Objects: 16 GB
FIRRTL Representation: 3.5 GB

When I invoke them in the same JVM, the elaborated Chisel graph sticks around the whole time. It's probably possible to fix them all, but due to this issue as well as the general notion that it is easier on people's existing build systems to keep Chisel and FIRRTL split, I decided to refocus on number 2. I am in the process of providing a translator from FIRRTL to ProtoBuf. The plan is to just implement FIRRTL<->Protobuf and use this PR to translate Chisel to FIRRTL for serialization.

For the record, the Chisel object is still massive but I suspect there are easy gains there. Even the FIRRTL representation has plenty of room for improvement.

At the moment, the Converter is very fast, but FIRRTL serialize is much slower than the Chisel Emitter

Also rename optName to optFile in dumpProto (didn't change dumpFirrtl because it is an existing public API)

Split out simple (1:1) Command -> Statement translations. Add comments to clarify how flat When Commands are converted to nested FIRRTL Conditionals

jackkoenig · 2018-06-28T22:54:54Z

I've refactored the style of the convert method for Commands to Statements (since that is by far the densest part). I split out conversion for Commands that map 1:1 to Statements from the when scope handling logic which is much more complicated. I also added clarifying comments to describe the basic process and noted things that I feel are non-obvious (or inconsistent). I also added Driver.dumpProto (which is why this is currently failing, it will pass once chipsalliance/firrtl#832 is merged).

I have also posted performance numbers in chipsalliance/firrtl#832. I backed out of using FIRRTL serialize because it is like 4x slower than the Chisel Emitter (as noted in chipsalliance/firrtl#838). However, the converter is still used by Driver.execute and Driver.dumpProto. I think speeding up FIRRTL serialization so that the Chisel Emitter can be removed is desirable but beyond the scope of this PR

jackkoenig · 2018-06-28T23:57:30Z

retest this please

jackkoenig · 2018-06-29T00:37:51Z

This is ready to go

azidar

Looks awesome! Great job @jackkoenig !

jackkoenig added 5 commits June 6, 2018 18:50

Initial version of Chisel IR to FIRRTL converter

03e43cc

Use Convert instead of Emitter

9fb2233

ULits with UnknownWidths should give minWidth in Converter

48bfeac

This works around an assumption in FIRRTL that Literals never have UnknownWidth

Change Driver.execute to pass the circuit to FIRRTL directly

abaac71

Make Driver a little DRYer

a0ea81e

jackkoenig requested review from chick, aswaterman, ducky64, ucbjrl, seldridge and azidar June 7, 2018 02:01

chick reviewed Jun 7, 2018

View reviewed changes

ducky64 reviewed Jun 7, 2018

View reviewed changes

jackkoenig added the DO NOT MERGE label Jun 7, 2018

jackkoenig mentioned this pull request Jun 8, 2018

Improve Parser and Visitor chipsalliance/firrtl#819

Merged

Convert commands to List in Converter, tail is slow on ArrayBuffer

6a5ccfd

seldridge reviewed Jun 14, 2018

View reviewed changes

jackkoenig added 2 commits June 27, 2018 20:22

Add Driver.dumpProto to directly emit protobuf

78fc4fc

Revert to using Emitter.emit instead of serialize on FIRRTL Circuit

d4acc34

At the moment, the Converter is very fast, but FIRRTL serialize is much slower than the Chisel Emitter

jackkoenig mentioned this pull request Jun 28, 2018

Serialization is a bit slow chipsalliance/firrtl#838

Open

jackkoenig added 3 commits June 28, 2018 14:30

Add ScalaDoc to dumpFirrtl and dumpProto

3842874

Also rename optName to optFile in dumpProto (didn't change dumpFirrtl because it is an existing public API)

Refactor Converter for Commands

a250e6e

Split out simple (1:1) Command -> Statement translations. Add comments to clarify how flat When Commands are converted to nested FIRRTL Conditionals

Merge branch 'master' into direct-to-firrtl

133908f

jackkoenig removed the DO NOT MERGE label Jun 29, 2018

Merge branch 'master' into direct-to-firrtl

f923209

azidar approved these changes Jul 2, 2018

View reviewed changes

Merge branch 'master' into direct-to-firrtl

31391c6

jackkoenig merged commit 3d8064a into master Jul 2, 2018

jackkoenig deleted the direct-to-firrtl branch July 2, 2018 22:27

jackkoenig added this to the 3.2.0 milestone Jul 13, 2018

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Direct to firrtl #829

Direct to firrtl #829

jackkoenig commented Jun 7, 2018 •

edited

Loading

chick Jun 7, 2018

chick Jun 7, 2018

jackkoenig Jun 7, 2018

jackkoenig Jun 7, 2018

jackkoenig commented Jun 7, 2018

ducky64 left a comment

ducky64 Jun 7, 2018

jackkoenig Jun 7, 2018

ducky64 Jun 7, 2018

jackkoenig Jun 7, 2018

seldridge left a comment

seldridge Jun 14, 2018

jackkoenig Jun 28, 2018

seldridge Jun 14, 2018

jackkoenig Jun 28, 2018

jackkoenig commented Jun 14, 2018 •

edited

Loading

jackkoenig commented Jun 28, 2018

jackkoenig commented Jun 28, 2018

jackkoenig commented Jun 29, 2018

azidar left a comment

Direct to firrtl #829

Direct to firrtl #829

Conversation

jackkoenig commented Jun 7, 2018 • edited Loading

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

jackkoenig commented Jun 7, 2018

ducky64 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

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

jackkoenig commented Jun 14, 2018 • edited Loading

jackkoenig commented Jun 28, 2018

jackkoenig commented Jun 28, 2018

jackkoenig commented Jun 29, 2018

azidar left a comment

Choose a reason for hiding this comment

jackkoenig commented Jun 7, 2018 •

edited

Loading

jackkoenig commented Jun 14, 2018 •

edited

Loading