Emit FIRRTL `BulkConnect`s for `Record`s, `Vec`s when possible #2073

debs-sifive · 2021-08-16T18:31:51Z

Contributor Checklist

Did you add Scaladoc to every public function/method?
Did you add at least one test demonstrating the PR?
Did you delete any extraneous printlns/debugging code?
Did you specify the type of improvement?
Did you add appropriate documentation in docs/src?
Did you state the API impact?
Did you specify the code generation impact?
Did you request a desired merge strategy?
Did you add text to be included in the Release Notes for this change?

cc @jackkoenig

Type of Improvement

backend code generation

API Impact

No visible impact to user.

Backend Code Generation Impact

FIRRTL BulkConnects (<-) are emitted when possible for Records and Vecs, instead of element-wise connections. This should decrease the FIRRTL emission size.

No impact on generated Verilog.

Desired Merge Strategy

Squash: The PR will be squashed and merged (choose this if you have no preference.

Release Notes

FIRRTL BulkConnects for Records, Vecs are emitted when possible, decreasing FIRRTL code emission size.

Reviewer Checklist (only modified by reviewer)

Did you add the appropriate labels?
Did you mark the proper milestone (3.2.x, 3.3.x, 3.4.x, 3.5.0) ?
Did you review?
Did you check whether all relevant Contributor checkboxes have been checked?
Did you mark as Please Merge?

debs-sifive · 2021-08-16T18:33:05Z

core/src/main/scala/chisel3/internal/MonoConnect.scala

@@ -178,6 +188,35 @@ private[chisel3] object MonoConnect {
      case (sink, source) => throw MismatchedException(sink.toString, source.toString)
    }

+  /** Check whether two [[Vec]]s can be bulk connected (<-) in FIRRTL. */
+  private[chisel3] def canBulkConnectVecs(sink_v: Vec[Data @unchecked],


This function and canBulkConnectRecords are also used in BiConnect. Is there a better place I should move these out to, or can they just live in MonoConnect?

mwachs5 · 2021-08-16T19:23:21Z

src/test/scala/chiselTests/Vec.scala

@@ -308,4 +308,19 @@ class VecSpec extends ChiselPropSpec with Utils {
        }}
    }
  }
+
+  property("Vec connections should emit FIRRTL bulk connects when possible") {


should we include some of the "when not possible" cases in the tests?

Yes we should

Some cases we should test:

import chisel3._ Bundle that instantiates an import Chisel._ Bundle that relies on the Stringly-typed connections for the connect to be legal, this should not emit a bulk connect. Some inspiration -- this example is a little weird because I'm surprised that out.buzz.foo <= in.buzz.foo in the emitted CHIRRTL uses <= instead of <-)

A case where <> will magically work but the FIRRTL types would be wrong for <=, eg. https://scastie.scala-lang.org/9xV1AAU0SFq1UvQLVbGFFw -- enq <= deq is illegal while Chisel can do the connecting by splitting it up.

Similar principles apply in Vecs, in fact you could make Vecs of these Bundles

core/src/main/scala/chisel3/internal/MonoConnect.scala

mwachs5 · 2021-08-16T19:25:21Z

core/src/main/scala/chisel3/internal/MonoConnect.scala

+      case _ => true
+    }
+    val elemsMatch = CheckTypes.validConnect(
+      Converter.extractType(sink_v, sourceInfo),


do the Vecs need to be parameterized exactly in order to be bulk connected? is that what is captured in extractType?

They need not be exact, implicit truncation remains the law of the land and that is correctly handled by validConnect.

@debs-sifive One thing that we also need to check is the Flows. The method needed for this is not exposed from FIRRTL so we might need to make some utility methods in FIRRTL: https://github.com/chipsalliance/firrtl/blob/4991ee209b4ae179afcb3e043bbe7ba92006931d/src/main/scala/firrtl/passes/CheckFlows.scala#L64. This is a bit tricky because those seem to require Kind and throw exceptions so we can't just reuse out of the box anyway

For development purposes, we can just write these utilities in Chisel and then migrate them to FIRRTL.

We also need to check "visibility" (eg. https://github.com/chipsalliance/chisel3/blob/b6929c9ad438db26055707b8ec5c66e4f70a22b8/core/src/main/scala/chisel3/internal/MonoConnect.scala#L217, need to make sure we're not issuing connects if one of the sides is not part of the current module).

mwachs5 · 2021-08-16T19:26:09Z

src/test/scala/chiselTests/MixedVecSpec.scala

+    })
+    chirrtl should include ("io.outMono <- io.inMono @[MixedVecSpec.scala")
+    chirrtl should include ("io.outBi <- io.inBi @[MixedVecSpec.scala")
+  }


It would be nice to see a case where the MixedVecs are parameterized differently. Can they be bulk connected in that case? If not it would be nice to see the check that they are NOT bulk connected.

src/test/scala/chiselTests/MixedVecSpec.scala

core/src/main/scala/chisel3/internal/MonoConnect.scala

debs-sifive · 2021-10-19T00:10:52Z

core/src/main/scala/chisel3/internal/MonoConnect.scala

@@ -178,6 +188,168 @@ private[chisel3] object MonoConnect {
      case (sink, source) => throw MismatchedException(sink.toString, source.toString)
    }

+  /** Check [[Aggregate]] visibility. */
+  private def aggregateConnectContextCheck(implicit sourceInfo: SourceInfo, connectCompileOptions: CompileOptions,


@jackkoenig I changed this to do aggregate-level checking, but am following the format of elemConnect(): https://github.com/chipsalliance/chisel3/blob/b6929c9ad438db26055707b8ec5c66e4f70a22b8/core/src/main/scala/chisel3/internal/MonoConnect.scala#L193

It's a lot of copy/paste code, but it seems like the elemConnect function was purposely written that way for clarity. Not sure if you recommend simplifying the code to be more straightforward?

Like from

if( (context_mod == sink_mod) && (context_mod == source_mod) ) { ((sink_direction, source_direction): @unchecked) match { // SINK SOURCE // CURRENT MOD CURRENT MOD case (Output, _) => true case (Internal, _) => true case (Input, _) => false } }

To:

if( (context_mod == sink_mod) && (context_mod == source_mod) ) { sink_direction != Input }

I wonder whether that will affect error message granularity - I found that the way it's laid out right now is nice for pointing out exactly which error condition was hit. Though I think it probably won't matter for this particular example when there's only one exception case

Ideally you should get the same error message — if we fail to emit a FIRRTL BulkConnect, we should try emitting as we did before (element-wise connect). And if that also fails, you should get the same element-wise failure message.

mwachs5 · 2022-02-07T17:05:48Z

Has this been replaced by #2381 , and shoudl this be closed?

debs-sifive · 2022-02-07T17:24:35Z

Yes, looks like it.

debs-sifive added 6 commits August 9, 2021 11:34

bulk connect records when possible

de5d332

fixing source/sink connection order

b6841ea

handling flipped record biconnects

f381fa8

bulk connect records passing all tests

d5a6b12

bulk connect for vecs

5158523

merge upstream

1985ea0

debs-sifive commented Aug 16, 2021

View reviewed changes

mwachs5 reviewed Aug 16, 2021

View reviewed changes

core/src/main/scala/chisel3/internal/MonoConnect.scala Outdated Show resolved Hide resolved

mwachs5 reviewed Aug 16, 2021

View reviewed changes

core/src/main/scala/chisel3/internal/MonoConnect.scala Outdated Show resolved Hide resolved

mwachs5 reviewed Aug 16, 2021

View reviewed changes

jackkoenig reviewed Aug 16, 2021

View reviewed changes

src/test/scala/chiselTests/MixedVecSpec.scala Outdated Show resolved Hide resolved

jackkoenig reviewed Aug 16, 2021

View reviewed changes

core/src/main/scala/chisel3/internal/MonoConnect.scala Outdated Show resolved Hide resolved

debs-sifive added 6 commits August 16, 2021 16:45

removing corresponds() call

a3bc6cf

workaround for MixedVec test

3075d60

switching to <= style strict bulk connects

cb2d21f

merge master

fb2d97d

applying stash to bumped code

0dbc9c3

revised aggregate connection checks

0ea0f0a

debs-sifive commented Oct 19, 2021

View reviewed changes

fixing context checks

fcab717

debs-sifive closed this Feb 7, 2022

debs-sifive deleted the bulkconnect branch May 12, 2022 18:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit FIRRTL `BulkConnect`s for `Record`s, `Vec`s when possible #2073

Emit FIRRTL `BulkConnect`s for `Record`s, `Vec`s when possible #2073

debs-sifive commented Aug 16, 2021

debs-sifive Aug 16, 2021

mwachs5 Aug 16, 2021

jackkoenig Aug 16, 2021

jackkoenig Aug 16, 2021

jackkoenig Aug 16, 2021

mwachs5 Aug 16, 2021

jackkoenig Aug 16, 2021

mwachs5 Aug 16, 2021

debs-sifive Oct 19, 2021

adkian-sifive Nov 4, 2021

debs-sifive Nov 5, 2021

mwachs5 commented Feb 7, 2022

debs-sifive commented Feb 7, 2022

Emit FIRRTL BulkConnects for Records, Vecs when possible #2073

Emit FIRRTL BulkConnects for Records, Vecs when possible #2073

Conversation

debs-sifive commented Aug 16, 2021

Contributor Checklist

Type of Improvement

API Impact

Backend Code Generation Impact

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

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

mwachs5 commented Feb 7, 2022

debs-sifive commented Feb 7, 2022

Emit FIRRTL `BulkConnect`s for `Record`s, `Vec`s when possible #2073

Emit FIRRTL `BulkConnect`s for `Record`s, `Vec`s when possible #2073