[FIRRTL] Add OMIR emission pass #2010

fabianschuiki · 2021-10-18T15:15:34Z

Add a pass to the FIRRTL dialect that consumes OMIRAnnotations on the circuit, gathers the tracker nodes back up, and serializes the data back into a JSON blob.

Todo

~~Replace wire/reg/inst names with NLA-based symbols ([LowertoHW] Lower NonLocalAnchor symbols with the VerbatimOp #2015)~~ (will be done in a follow-up PR)
Add FIR-to-SV/JSON integration test

fabianschuiki · 2021-10-20T05:49:54Z

I think this is ready for a first round of reviews. The EmitOMIR pass is supposed to run right before LowerToHW. It gathers all OMIRAnnotations on the circuit and converts them back to JSON. In the process, it gathers the tracker annotations scattered during read-in and substitutes them back into the various OM.+Target nodes.

At this point it uses symbols to interpolate module names in the final JSON, but hardcodes wire/reg/regreset/inst names until #2015 lands.

darthscsi

Seems fine. I'll let @seldridge comment on any OMIR specifics.

include/circt/Dialect/FIRRTL/Passes.h

darthscsi · 2021-10-20T14:43:51Z

include/circt/Dialect/FIRRTL/Passes.td

+    reading the OMIR, and serializes the resulting data into a JSON file.
+  }];
+  let constructor = "circt::firrtl::createEmitOMIRPass()";
+  let options = [Option<"outputFilename", "file", "std::string", "",


This pass only makes sense with an output file, yes?

Right now there's still a OMIRFileAnnotation, which we probably want to get rid of in favor of command line args.

lib/Dialect/FIRRTL/Transforms/EmitOMIR.cpp

darthscsi · 2021-10-20T14:52:44Z

lib/Dialect/FIRRTL/Transforms/EmitOMIR.cpp

+      orderedFields.push_back({index, nameAndField.first, fieldDict});
+    }
+    llvm::sort(orderedFields,
+               [](auto a, auto b) { return std::get<0>(a) < std::get<0>(b); });


std::less would work too (though a bit more constrained)

That was my first reflex as well, but gcc complained about not being able to compare Identifier and DictionaryAttr properly.

darthscsi · 2021-10-20T14:53:59Z

tools/firtool/firtool.cpp

@@ -111,6 +111,10 @@ static cl::opt<bool>
                 cl::desc("emit metadata for metadata annotations"),
                 cl::init(true));

+static cl::opt<bool>
+    emitOMIR("emit-omir", cl::desc("emit OMIR to JSON metadata annotations"),


Bike shedding: maybe reserve "emit" for things which produce files?

It does, like emit-metadata. I've messed up the cl::desc here which makes it sound like it's producing yet another annotation, when in fact it's producing an sv.verbatim blob that goes into a JSON file.

One approach would be to always turn this on if the user passes in an OMIR file.

It's now on-by-default, and if there's no OMIRAnnotation, nothing happens. I think that should cover what you have in mind?

darthscsi · 2021-10-20T14:58:20Z

test/Dialect/FIRRTL/emit-omir.mlir

+  firrtl.nla @nla_0 [@NonLocalTrackers, @B, @A] ["b", "a", "A"]
+  firrtl.module @A() attributes {annotations = [{circt.nonlocal = @nla_0, class = "freechips.rocketchip.objectmodel.OMIRTracker", id = 0}]} {}
+  firrtl.module @B() {
+    firrtl.instance a {annotations = [{circt.nonlocal = @nla_0, class = "circt.nonlocal"}]} @A()


would this need a don't touch in the output so that it keeps the name "a"? There may be some basic sv.verbatim lowering that should happen in lowertohw so that nlas can be converted to symbols in a general way and sv.verbatim nodes earlier can pass an nla in to name something.

If we don't want to break it, then it would need the weaker don't touch variant of "keep me around, but don't block constant propagation". This doesn't exist yet, though. 😢

Same question about a and b below. Is the intent that this will be fixed once Prithayan's stuff lands?

Yeah good point. There probably have to be a bunch of "don't touch"-annos coming out of EmitOMIR. Ideally they would only be "don't inline" since renaming of the instances is actually okay (as long as the NLAs are in place).

Even "more ideally", you would also be able to change the hierarchy around. But that would require us to have a way of controlling how NLAs get interpolated into sv.verbatim. Because for an SV hierarchical name you might want $root.a.b.c, but for an OMIR output you'd want a Chisel-style ~Circuit|Module/a:Foo/b:Bar/c:Baz.

Maybe something like a dedicated NLA-as-XMR formatting op could work?

%0 = sv.nla_path @nla_0 {format = "sv"} : !sv.path %1 = sv.nla_path @nla_0 {format = "chisel"} : !sv.path sv.verbatim "sv {{0}}; chisel {{1}}"(%0, %1)

@seldridge My current plan is to create an NLA for every element in the path, and then use the work @prithayan is doing to assemble the full path. That should at least allow for renaming of a and b.

seldridge

This looks awesome, Fabian. Thank you!

Nitty comments throughout. I did not dig into the logic serializing the OMIR too deeply, but this is relatively straightforward. I am annoyed that we have all this duplicated logic for verifying the OMIR during parsing and then here. Not sure what to do about that, though, other than eventually moving to actual custom attributes.

seldridge · 2021-10-20T15:24:02Z

lib/Dialect/FIRRTL/Import/FIRAnnotations.cpp

+    fields.append("omir.tracker", UnitAttr::get(ctx));
    fields.append("type", StringAttr::get(ctx, tpe));


Is adding this singleton strictly necessary?

Note: I like omir.tracker better than the "freechips.rocketchip.objectmodel.OMIRTracker" which I made up for the scattered targets.

The problem here is that when you see {type = "asdf", id = 42} in the value field (or nested within it), it's not clear if this is a OMMap with fields "type" and "id", or if this used to be a OM.+Target. Adding the omir.tracker unit is an unambiguous marker that the dictionary is actually a OM.+Target, because there's nothing in the original OMIR input JSON that can generate such a unit attr.

Also, for the OMIRTracker annotations it's important that we still have a "class" field, since that's what we implicitly require for the annotations array on operations.

seldridge · 2021-10-20T15:26:52Z

test/Dialect/FIRRTL/emit-omir.mlir

+// CHECK-SAME:  \22name\22:\22OMBoolean\22,\22value\22:true
+// CHECK-SAME:  \22name\22:\22OMInt1\22,\22value\22:9001
+// CHECK-SAME:  \22name\22:\22OMInt2\22,\22value\22:-42
+// CHECK-SAME:  \22name\22:\22OMDouble\22,\22value\22:3.14
+// CHECK-SAME:  \22name\22:\22OMID\22,\22value\22:\22OMID:1337\22
+// CHECK-SAME:  \22name\22:\22OMReference\22,\22value\22:\22OMReference:0\22
+// CHECK-SAME:  \22name\22:\22OMBigInt\22,\22value\22:\22OMBigInt:42\22
+// CHECK-SAME:  \22name\22:\22OMLong\22,\22value\22:\22OMLong:ff\22
+// CHECK-SAME:  \22name\22:\22OMString\22,\22value\22:\22OMString:hello\22
+// CHECK-SAME:  \22name\22:\22OMBigDecimal\22,\22value\22:\22OMBigDecimal:10.5\22
+// CHECK-SAME:  \22name\22:\22OMDeleted\22,\22value\22:\22OMDeleted:\22
+// CHECK-SAME:  \22name\22:\22OMConstant\22,\22value\22:\22OMConstant:UInt<2>(\\\22h1\\\22)\22
+// CHECK-SAME:  \22name\22:\22OMArray\22,\22value\22:[true,9001,\22OMString:bar\22]
+// CHECK-SAME:  \22name\22:\22OMMap\22,\22value\22:{\22bar\22:9001,\22foo\22:true}


It's annoying that we get all these \22 codes instead of being able to look at quotes here. One idea would be to pipe the output through sed or something (though that introduces a dependency on sed being installed) to make the test more readable.

Yeah this is very painful to look at 😢. It's much better in the integration test, because that can look at the ExportVerilog result, but we'll want some form of single-pass unit test around as well.

seldridge · 2021-10-20T15:29:20Z

test/Dialect/FIRRTL/emit-omir.mlir

+  firrtl.nla @nla_0 [@NonLocalTrackers, @B, @A] ["b", "a", "A"]
+  firrtl.module @A() attributes {annotations = [{circt.nonlocal = @nla_0, class = "freechips.rocketchip.objectmodel.OMIRTracker", id = 0}]} {}
+  firrtl.module @B() {
+    firrtl.instance a {annotations = [{circt.nonlocal = @nla_0, class = "circt.nonlocal"}]} @A()


If we don't want to break it, then it would need the weaker don't touch variant of "keep me around, but don't block constant propagation". This doesn't exist yet, though. 😢

seldridge · 2021-10-20T15:35:27Z

test/Dialect/FIRRTL/emit-omir.mlir

+// CHECK-LABEL: firrtl.circuit "NonLocalTrackers" {
+// CHECK:       sv.verbatim
+// CHECK-SAME:  \22name\22:\22OMReferenceTarget1\22,\22value\22:\22OMReferenceTarget:~NonLocalTrackers|{{[{][{]0[}][}]}}/b:{{[{][{]1[}][}]}}/a:{{[{][{]2[}][}]}}\22
+// CHECK-SAME:  #hw.output_file<"omir.json", excludeFromFileList>
+// CHECK-SAME:  symbols = [@NonLocalTrackers, @B, @A]


Test output here looks great!

seldridge · 2021-10-20T15:36:39Z

test/Dialect/FIRRTL/emit-omir.mlir

+  firrtl.nla @nla_0 [@NonLocalTrackers, @B, @A] ["b", "a", "A"]
+  firrtl.module @A() attributes {annotations = [{circt.nonlocal = @nla_0, class = "freechips.rocketchip.objectmodel.OMIRTracker", id = 0}]} {}
+  firrtl.module @B() {
+    firrtl.instance a {annotations = [{circt.nonlocal = @nla_0, class = "circt.nonlocal"}]} @A()


Same question about a and b below. Is the intent that this will be fixed once Prithayan's stuff lands?

lib/Dialect/FIRRTL/Transforms/EmitOMIR.cpp

seldridge · 2021-10-20T16:27:25Z

lib/Dialect/FIRRTL/Transforms/EmitOMIR.cpp

+    return;
+  }
+
+  // Extract and order the fields of this node.


Nice catch about remembering to order this!

Would have been a shame if your index field went to waste 😄

seldridge · 2021-10-20T17:30:20Z

lib/Dialect/FIRRTL/Transforms/EmitOMIR.cpp

+    jsonStream.attributeBegin("value");
+    emitValue(field.get("value"), jsonStream);
+    jsonStream.attributeEnd();


I hadn't noticed these attributeBegin/attributeEnd member functions. Nice.

Yeah the JSON API is very pleasant to work with, I'm genuinely impressed 😄

lib/Dialect/FIRRTL/Transforms/EmitOMIR.cpp

Add a pass to the FIRRTL dialect that consumes `OMIRAnnotation`s on the circuit, gathers the tracker nodes back up, and serializes the data back into a JSON blob.

fabianschuiki · 2021-10-21T18:26:34Z

Thanks for the reviews @darthscsi and @seldridge. Going to merge this now to unblock follow-up work.

fabianschuiki added the FIRRTL Involving the `firrtl` dialect label Oct 18, 2021

fabianschuiki force-pushed the emit-omir branch 2 times, most recently from 5fef5d2 to ac0ea9e Compare October 18, 2021 17:41

fabianschuiki changed the title ~~[FIRRTL] Add OMIR emission pass~~ WIP: [FIRRTL] Add OMIR emission pass Oct 19, 2021

fabianschuiki force-pushed the emit-omir branch 3 times, most recently from 545b124 to 45e2c83 Compare October 19, 2021 18:42

fabianschuiki changed the title ~~WIP: [FIRRTL] Add OMIR emission pass~~ [FIRRTL] Add OMIR emission pass Oct 20, 2021

fabianschuiki requested a review from seldridge October 20, 2021 05:46

fabianschuiki marked this pull request as ready for review October 20, 2021 05:46

fabianschuiki requested review from darthscsi and prithayan October 20, 2021 05:50

darthscsi approved these changes Oct 20, 2021

View reviewed changes

darthscsi reviewed Oct 20, 2021

View reviewed changes

fabianschuiki force-pushed the emit-omir branch from 45e2c83 to 60b318d Compare October 20, 2021 16:18

seldridge approved these changes Oct 20, 2021

View reviewed changes

fabianschuiki force-pushed the emit-omir branch from 60b318d to bbc3de6 Compare October 21, 2021 16:53

[FIRRTL] Add OMIR emission pass

d1069c8

Add a pass to the FIRRTL dialect that consumes `OMIRAnnotation`s on the circuit, gathers the tracker nodes back up, and serializes the data back into a JSON blob.

fabianschuiki force-pushed the emit-omir branch from bbc3de6 to d1069c8 Compare October 21, 2021 17:49

fabianschuiki merged commit 4cd3709 into llvm:main Oct 21, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FIRRTL] Add OMIR emission pass #2010

[FIRRTL] Add OMIR emission pass #2010

fabianschuiki commented Oct 18, 2021 •

edited

Loading

fabianschuiki commented Oct 20, 2021

darthscsi left a comment

darthscsi Oct 20, 2021

fabianschuiki Oct 20, 2021

darthscsi Oct 20, 2021

fabianschuiki Oct 20, 2021

darthscsi Oct 20, 2021

fabianschuiki Oct 20, 2021

seldridge Oct 20, 2021

fabianschuiki Oct 20, 2021

darthscsi Oct 20, 2021

seldridge Oct 20, 2021

seldridge Oct 20, 2021

fabianschuiki Oct 20, 2021

fabianschuiki Oct 20, 2021

seldridge left a comment

seldridge Oct 20, 2021

fabianschuiki Oct 20, 2021

fabianschuiki Oct 20, 2021 •

edited

Loading

seldridge Oct 20, 2021

fabianschuiki Oct 20, 2021

seldridge Oct 20, 2021

seldridge Oct 20, 2021

seldridge Oct 20, 2021

seldridge Oct 20, 2021

fabianschuiki Oct 20, 2021 •

edited

Loading

seldridge Oct 20, 2021

fabianschuiki Oct 20, 2021

fabianschuiki commented Oct 21, 2021

		fields.append("omir.tracker", UnitAttr::get(ctx));
		fields.append("type", StringAttr::get(ctx, tpe));

[FIRRTL] Add OMIR emission pass #2010

[FIRRTL] Add OMIR emission pass #2010

Conversation

fabianschuiki commented Oct 18, 2021 • edited Loading

Todo

fabianschuiki commented Oct 20, 2021

darthscsi 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

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

fabianschuiki Oct 20, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianschuiki Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabianschuiki commented Oct 21, 2021

fabianschuiki commented Oct 18, 2021 •

edited

Loading

fabianschuiki Oct 20, 2021 •

edited

Loading

fabianschuiki Oct 20, 2021 •

edited

Loading