Add instance inline API #838

seldridge · 2018-06-22T18:56:33Z

This adds a circuit/module/instance inlining and flattening API. This adds three new traits:

InlineInstance that will inline any Module/Instance it's mixed into
FlattenInstance that will flatten any Module/Instance it's mixed into

This provides Chisel-level access to existing FIRRTL annotations (InlineAnnotation and FlattenAnnotation) that map to existing FIRRTL passes/transforms (InlineInstances and Flatten).

Related issue:
Resolves #602

Type of change: other enhancement

Impact: API addition (no impact on existing code)

Development Phase: implementation

Release Notes

Add instance inlining and flattening API

jackkoenig · 2018-06-30T00:55:18Z

src/main/scala/chisel3/util/Inline.scala

+  *  }
+  * }}}
+  */
+trait InlineInstance { self: BaseModule =>


I think the trait is useful to inline all instances of a given Module, do you think we could also have a way to inline a single instance?

Oh, I see you do it in the example! Nice :)

aswaterman · 2018-07-04T04:23:25Z

I was just about to add something similar to rocket-chip before discovering this PR. (In fact, I was going to propose syntax identical to Module(new Bar with InlineInstance).) So, LGTM.

jackkoenig

Looks really good overall, a couple of questions/comments

jackkoenig · 2018-07-07T00:19:44Z

src/main/scala/chisel3/util/Inline.scala

+  def inlineComponent(m: BaseModule): Unit = chisel3.experimental.annotate(
+    new ChiselAnnotation with RunFirrtlTransform {
+      def toFirrtl = InlineAnnotation(ComponentName(m.instanceName, self.toNamed))
+      def transformClass = classOf[InlineInstances] }


What is the utility of this alternative way of specifying that an instance should be inlined? I guess you could make a submodule in code you don't control or don't want to modify inline an instance.

Had to think about this as I didn't have good justification originally. If a library writer chooses to only expose a wrapped Module via a factory then this would be the only way to inline that instance. Consider:

object BarFactory { def apply(): Bar = Module(new Bar) } class Top extends Module with InlineHelpers { val io = IO(new Bundle{ val a = Input(Bool()) }) val Seq(x, y, z) = Seq.fill(3)(BarFactory()) Seq(x, y, z).map(_.io.a := io.a) inlineComponent(x) }

There's nothing that I'm aware of where this type of API is the only one that is exposed (something like chisel3.util.Queue is generally used only via factory, but the underlying class is public). Nonetheless, there may be some motivation for only exposing a factory and causing that.

Do you think this is worth including or is the case here too pathological?

As a user I think it would be good to be able to inline a module created in a 3rd-party library without having to modify that library (promotes re-use instead of fork/copy-paste).

Sorry I missed this 10 days ago.

I agree that we would like to be able to inline modules constructed in factories methods, but in practice this won't work for that either. As in the case of Queue, factory methods for constructing modules typically don't return the actual module reference itself, rather they return an interface (eg. see object Queue).

I think we need to come up with another way to support that in the general case. One idea I (or @aswaterman, I can't remember who proposed it) had was to provide a mechanism similar to withClock where you could scope things below to be inlined. There could be something like inlineInstances { } or the like. Unfortunately, this would require some form of support from Chisel, either natively or some API to get references to the Modules instantiated within a block.

In any case, I think we should remove this API and just have the mixin.

Yeah, Queue was only intending to be illustrative of how a user might write a library (and hopefully pass back the module as opposed to the IO). I like the idea of inlineInstances { }, but yes, that's likely out of scope here.

As API deprecation is such a hot topic, I think it's best to err with new APIs on the side of caution. I'll remove this.

jackkoenig · 2018-07-07T00:20:14Z

src/test/scala/chiselTests/InlineSpec.scala

+  class Foo extends Module with Internals with InlineHelpers with InlineInstance
+  class Bar extends Module with Internals with InlineHelpers
+
+  def collectInstances(c: fir.Circuit): Array[WDefInstance] = {


Why Array? It's not all that important but usually we just use Seq

Was due to the internal use of a mutable ArrayBuffer, but that gets converted automatically. I'll switch the internal to a ListBuffer as that's all that needed and return a Seq. Consistency is good...

jackkoenig · 2018-07-07T00:21:30Z

src/test/scala/chiselTests/InlineSpec.scala

+      m
+    }
+    c map onModule
+    insts.toArray.map{ case WDefInstance(_, n, m, _) => WDefInstance(fir.NoInfo, n, m, fir.UnknownType) }


Alternatively:

insts.toArray.map(inst => inst.copy(info = fir.NoInfo, tpe = fir.UnknownType))

Much cleaner, thanks! I can even shave off the inst => inst.copy --> _.copy ...

jackkoenig · 2018-07-07T00:27:38Z

src/main/scala/chisel3/util/Inline.scala

+    * }
+    * }}}
+    */
+  def inlineCircuit(): Unit = chisel3.experimental.annotate(


This one feels kind of dangerous. I dunno how I feel about an arbitrary point in the Module hierarchy being able to instigate inlining all modules. I would maybe make InlineCircuit as trait and have it error unless it's mixed in to the top module. Alternatively, I think Flatten in FIRRTL inlines everything below a given module? Maybe make a FlattenHierarchy trait that can be mixed in to flatten everything below and thus Inlining the whole circuit can be accomplished by mixing that in at the top?

I personally don't like it either... This PR only includes that because the FIRRTL inlining annotations support a CircuitName that will flatten the whole thing. I was struggling with a good API for that at the Chisel level (and will usually aim to expose whatever FIRRTL API exists at the Chisel level). I like the approach of requiring that this be mixed into the top module if that is the desired effect. Or: that's much better than "spooky inlining at a distance..."

This does bring up a point, which did not occur to me, that a Chisel flattening API should be a part of the inlining API (since they're both frontends for InlineInstances). Are you in agreement that that would round out this PR?

Let me check that I'm understanding correctly but I think I agree.

By Chisel flattening API do you mean some API in Chisel (eg. trait FlattenHierarchy) to flatten everything below the given module? And in adding that, we would remove def inlineCircuit()?

(Sorry, dropped off on this...)

Yes, I mean an API for flattening everything below a given module (an annotator that emits a firrtl.transforms.FlattenAnnotation which will run firrtl.transforms.Flatten should be all that's needed). I'm fine with removing the inlineCircuit API as it seems at worst dangerous (somebody uses this in their library) and at best confusing (SO posts asking, "Why is my circuit inlined?"). The initial motivation for inlineCircuit was only to fully expose the entire FIRRTL API of firrtl.passes.Inline, but it's far better to expose a usable sane Chisel API.

seldridge · 2018-07-18T03:51:21Z

@jackkoenig: this should be good for another look. I added a trait and a method for accessing firrtl.transforms.Flatten as it's natural to include this here (since flattening is a wrapper around firrtl.passes.InlineInstances) and it seems to have been implicitly needed from your comments about circuit inlining. Related to circuit inlining, I removed the spooky inlineCircuit method.

jackkoenig · 2018-07-27T23:03:31Z

src/main/scala/chisel3/util/Inline.scala

+  def inlineComponent(m: BaseModule): Unit = chisel3.experimental.annotate(
+    new ChiselAnnotation with RunFirrtlTransform {
+      def toFirrtl = InlineAnnotation(ComponentName(m.instanceName, self.toNamed))
+      def transformClass = classOf[InlineInstances] }


Sorry I missed this 10 days ago.

I agree that we would like to be able to inline modules constructed in factories methods, but in practice this won't work for that either. As in the case of Queue, factory methods for constructing modules typically don't return the actual module reference itself, rather they return an interface (eg. see object Queue).

I think we need to come up with another way to support that in the general case. One idea I (or @aswaterman, I can't remember who proposed it) had was to provide a mechanism similar to withClock where you could scope things below to be inlined. There could be something like inlineInstances { } or the like. Unfortunately, this would require some form of support from Chisel, either natively or some API to get references to the Modules instantiated within a block.

In any case, I think we should remove this API and just have the mixin.

jackkoenig · 2018-07-27T23:04:52Z

src/main/scala/chisel3/util/Inline.scala

+    * }
+    * }}}
+    */
+  def flattenComponent(m: BaseModule): Unit = chisel3.experimental.annotate(


I don't love this API. It's not the worst thing in the world (it's similar to dontTouch) so I don't think it's worth blocking over but similarly to prefering some sort of inlineInstances { }-like API maybe there could be a flattenSubModules { } or something, of course it would require support in Chisel core

If we keep it, I think we should make it return the argument itself and probably rename it to flattenHierarchy or something which IMO is more clear about what it's doing

I'll remove this as well. If a user wants this behavior, they can write their own annotator. Otherwise, we should be absolutely certain that the API we expose publicly is the one we want to support. Keeping this back in favor of a future better API makes more sense than pushing this out and then deprecating or supporting both.

seldridge · 2018-07-28T02:24:24Z

@jackkoenig: After your comments and the discussion today about the need to not deprecate APIs willy-nilly, I opted to remove the questionable APIs and only provide the mixin APIs that everyone (you, @aswaterman, me) seems to like. I additionally, updated the documentation to be clear about what's going on and to do the testing more vigorously. For testing, the actual instance names are checked.

We can move forward with the inlineInstances { } API at some point. I'm in agreement that this is better than the dontTouch method style annotators.

This should be good for another look.

jackkoenig

lgtm!

jackkoenig · 2018-08-21T00:40:50Z

retest this please

seldridge · 2018-08-21T03:42:45Z

No clue what's going on with Jenkins. Is it applying more stringent scalastyle checks than are in the public XML? Since I was taking a look at this anyway to make sure it was passing tests/scalastyle, I went ahead and rebased it into two logical commits that both build.

One question: should we move this into chisel3.util.experimental like BoringUtils?

jackkoenig · 2018-08-21T18:23:21Z

Yeah I think it would be better in chisel3.util.experimental, if you want to rebase it again, I can then do a merge commit after?

seldridge · 2018-08-21T18:28:35Z

Sounds good to me. Taking care of the rebase now.

ucbjrl · 2018-08-21T18:40:07Z

Jenkins checkstyle is configured to complain with 40 or more style violations.

seldridge · 2018-08-21T18:41:46Z

@ucbjrl, thanks! That clarifies things!

jackkoenig · 2018-08-23T18:04:10Z

@seldridge Do you want to rebase this (again)? Looks like another PR sneaked in ahead

This adds a new trait, InlineInstance, to chisel3.util.experimental. This trait, when mixed into a specific module or instance, will "inline" that module, i.e., "collapse a module while preserving it's submodules." This includes testing (InlineSpec) and ScalaDoc documentation. Signed-off-by: Schuyler Eldridge <[email protected]>

This adds a new trait, FlattenInstance, to chisel3.util.experimental. When mixed into a module or a specific instance this trait will "flatten", i.e., "inline that module and all of its submodules". This includes testing (additions to InlineSpec) and ScalaDoc documentation. Signed-off-by: Schuyler Eldridge <[email protected]>

seldridge · 2018-08-23T18:08:40Z

@jackkoenig: rebased and building. Thanks for the ping. It would be nice if GitHub had a "rebase and then create a merge commit" option...

jackkoenig · 2018-08-23T18:09:42Z

Yeah for real, or if CI could rerun a PR against the new master automatically so that it would be fully safe to just do a merge

jackkoenig self-requested a review June 29, 2018 20:39

jackkoenig reviewed Jun 30, 2018

View reviewed changes

aswaterman added the Feature New feature, will be included in release notes label Jul 4, 2018

jackkoenig added this to the 3.2.0 milestone Jul 6, 2018

jackkoenig requested changes Jul 7, 2018

View reviewed changes

seldridge force-pushed the issue-602 branch 2 times, most recently from bc96990 to 098758b Compare July 18, 2018 03:43

jackkoenig reviewed Jul 27, 2018

View reviewed changes

jackkoenig approved these changes Aug 17, 2018

View reviewed changes

seldridge force-pushed the issue-602 branch 2 times, most recently from ae9c203 to 55110d9 Compare August 21, 2018 03:39

seldridge force-pushed the issue-602 branch from 55110d9 to ec86557 Compare August 21, 2018 18:37

seldridge force-pushed the issue-602 branch from 5d6c211 to 28a4f09 Compare August 21, 2018 20:43

seldridge added 2 commits August 23, 2018 14:05

seldridge force-pushed the issue-602 branch from a17b323 to 87551bf Compare August 23, 2018 18:05

seldridge force-pushed the issue-602 branch from 87551bf to ee0f27c Compare August 23, 2018 18:11

jackkoenig merged commit 52daa37 into chipsalliance:master Aug 23, 2018

seldridge deleted the issue-602 branch August 23, 2018 18:45

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

seldridge mentioned this pull request Sep 18, 2019

Attributes chipsalliance/firrtl#1172

Closed

kammoh mentioned this pull request Apr 2, 2021

Top level InlineInstance crashes FIRRTL #1851

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add instance inline API #838

Add instance inline API #838

seldridge commented Jun 22, 2018 •

edited

Loading

jackkoenig Jun 30, 2018

jackkoenig Jun 30, 2018

aswaterman commented Jul 4, 2018

jackkoenig left a comment

jackkoenig Jul 7, 2018

seldridge Jul 18, 2018

edwardcwang Jul 18, 2018

jackkoenig Jul 27, 2018

seldridge Jul 28, 2018

jackkoenig Jul 7, 2018

seldridge Jul 18, 2018

jackkoenig Jul 7, 2018

seldridge Jul 18, 2018

jackkoenig Jul 7, 2018

seldridge Jul 7, 2018

jackkoenig Jul 10, 2018

seldridge Jul 17, 2018

seldridge commented Jul 18, 2018

jackkoenig Jul 27, 2018

jackkoenig Jul 27, 2018

jackkoenig Jul 27, 2018

seldridge Jul 28, 2018

seldridge commented Jul 28, 2018

jackkoenig left a comment

jackkoenig commented Aug 21, 2018

seldridge commented Aug 21, 2018

jackkoenig commented Aug 21, 2018

seldridge commented Aug 21, 2018

ucbjrl commented Aug 21, 2018

seldridge commented Aug 21, 2018

jackkoenig commented Aug 23, 2018

seldridge commented Aug 23, 2018

jackkoenig commented Aug 23, 2018

Add instance inline API #838

Add instance inline API #838

Conversation

seldridge commented Jun 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aswaterman commented Jul 4, 2018

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

seldridge commented Jul 18, 2018

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 commented Jul 28, 2018

jackkoenig left a comment

Choose a reason for hiding this comment

jackkoenig commented Aug 21, 2018

seldridge commented Aug 21, 2018

jackkoenig commented Aug 21, 2018

seldridge commented Aug 21, 2018

ucbjrl commented Aug 21, 2018

seldridge commented Aug 21, 2018

jackkoenig commented Aug 23, 2018

seldridge commented Aug 23, 2018

jackkoenig commented Aug 23, 2018

seldridge commented Jun 22, 2018 •

edited

Loading