Create RecordMap for better diplomatic I/O naming

[mwachs5]

Optionally allow HeterogeneousBag signals to be named something other than "_0", "_1"...
Also allow HeterogenousBag signals to be indexed by such names.
Create a RecordMap as an alternative to HeterogenousBag with explicit names for the elements.

This PR does not yet use this API anywhere, but I plan to make it use it for clock bundles as a demonstration, likely in a follow-on PR (see #2487 )

Related issue: None

Type of change: feature request

Impact: API addition (no impact on existing code)

Development Phase: implementation

Release Notes

Add a RecordMap that allows names to be specified along with the Data elements for better. signal naming. This would result in verilog with different signal names if applied.

[jackkoenig]

Is it better to add this optional API to HeterogeneousBag vs. creating a new type? I'm guessing the issue is that we hardcode HetBag throughout rocket-chip, but I believe this is more of a historic need than something we should have now.

IIRC, HetBag originally existed to make it possible to construct IOs from Chisel objects that already existed and were already connected to. This is because it predates MultiIOModule and RawModule so it made it possible to use the cake pattern, connecting to IOs, and yet later group them up into a single HetBag val io. This pattern is now much better served by using MultiIOModule and even chisel3.experimental.IO for arbitrary programmatic IO materialization. I suspect HetBag is no longer serving it's original purpose, but it remains pervasive in the API. (Also note that this old pattern will not work in import chisel3._ code anyway, it only works if you don't require a binder operation like IO(...) to manifest hardware).

My suspicion, is that we simply should stop mandating HetBag throughout, instead using merely T <: Data, then provide a different subclass of Record that has user-provided names, perhaps RecordMap or CustomBundle. This type could conceivably even be integrated into chisel3.util, there is a more modern of HeterogeneousBag there called MixedVec.

[jackkoenig]

TLDR of my above, this PR is kind of like adding a names field to a type like List because you want the List to act like a Map, when I think the better approach would be to use a different data structure, like a Map.

[mwachs5]

I don't think there is any mandation (?) of using HeterogenousBag. Now that I implemented an example, I realize the different diplomatic classes choose to use it for creating their Bundles, but since those diplomatic classes have to be modified to pass names in anyway, you're right that it could just do something other than putting down a HeterogenousBag. As long as the API for whatever it is you do with the HeterogenousBag (e.g. connecting to some other Seq with zips and assuming stuff is in the expected order) still does the right thing.

I thought about creating a new class but seemed like that would be less DRY than adding to this, but it does seem awkward to pass in two lists of things instead of a Map, like you say.

[hcook]

Thinking back, my original concept was indeed to make a new class, but that got lost somewhere in the jira creation process. I agree with @jackkoenig 's feedback about making this a potentially upstream-able contribution, and @mwachs5 is right that we can roll it out as we use it.

@jackkoenig A related refactoring that would "clean up" a lot of usages is to switch things to makeIOs while also changing the implementation of that. That change plus relaxing some arguments to be Record would be a good followup.

[mwachs5]

Ok, by "upstreamable" you mean "part of chisel itself"?
I think the concrete suggestion is to make a new subclass of Record, RecordMap[T], that takes a Map[String, Data <: T] and turns that into the mapping.

@hcook Is it important to the API that the items are still ordered properly so they can be zipped up together/ indexed? So should this be explicitly RecordListMap[T]? I find it unlikely that people are always going to want to go by name or might screw up zipping things together that don't end up in the right order.

[hcook]

Yes, I meant "upstream into chisel itself"

I think further enforcement of ordering is never a bad choice, so ListMap seems appropriate.

It it interesting to think about how one might use the + and - operators to generate new Records as a result of a circuit that does some filtering or adds extra information.

Also, as future work, I think we could consider using selectDynamic to provide . operator accessors to the members

[jackkoenig]

I have some concerns about extending collection.Map and what exactly it means.

[jackkoenig]

More thoughts. HeterogeneousBag provides some dangerous patterns that I think we should eschew in new code.

[jackkoenig]

I don't think we should extend Seq, I think the only thing we should extend is Record. Seq is less problematic than Map, but there is the issue that it may be surprising.

[mwachs5]

so the common pattern is to want to zip these things together. Can I still do that if it doesnt' extend Seq?

[mwachs5]

though there is an argument to be made that zipping them up sequentially (vs pairing them by name) is exactly the buggy case we want to avoid... thoughts @hcook ?

[jackkoenig]

.elements is of type ListMap which implements Iterable so you can always zip those, eg.

whatever.zip(myRecordMap.elements)

[mwachs5]

To be clear... elements is not the REAL elements, right? Given your suggestion below, elements returns some sort of cloned type version thing, wheras eltMap.values gives... the real data?

I wrote a val data = eltMap.values function separately from elements because it seemed more correct/different for the use case when you actually want to connect to the bundle, vs clone its type. But I'm having difficulty getting this right.

I also made eltMap public.

[jackkoenig]

Suggested change

	val elements = eltMap
	override def cloneType: this.type = (new RecordMap(eltMap.map{case (k, v) => k -> v.chiselCloneType})).asInstanceOf[this.type]
	import chisel3.internal.requireIsChiselType
	import chisel3.experimental.internal.chiselTypeClone
	eltMap.foreach { case (name, elt) => requireIsChiselType(elt, name) }
	val elements = ListMap() ++ eltMap.mapValues(chiselTypeClone) // mapValues return value is lazy
	override def cloneType: this.type = (new RecordMap(eltMap)).asInstanceOf[this.type]

I know this is following the style of HeterogeneousBag, but I think this is wrong. In general, custom Record implementations should provide fresh types to elements rather than the ones passed by the user. This is because the elements must be bound in place by Chisel, ie. they will be mutated. HeterogeneousBag did the "copy on clone" technique because it was intentionally allowing things to be used as hardware before the HeterogeneousBag was created. This was because all ports had to be packed into val io. Since that is no longer true, this pattern should no longer be used. Furthermore, the pattern won't work in import chisel3._ anyway.

This suggestion also implies that we could make the eltMap argument an Iterable[String, T] and then provide a companion object with a varargs version as well.

[mwachs5]

I tried the Iterable[String, T] but then that was annoying to write an accessor by the string name, which I assume we would want at some point. I still wrote the varargs version.

I did not use your suggestion directly, but I believe I captured the request... please take a look.

This doesnt work in my use case however, I get an exception on the chiselTypeClone call. WIll point it out on #2487

[hcook]

def apply[T <: Data](elements: (String, T)*): RecordMap[T] = {?

[mwachs5]

sorry are you just suggesting that I add the return type to the function declaration? If so I agree

[mwachs5]

Suggested change

	def apply[T <: Data](eltMap: ListMap[String, T]) = new RecordMap(eltMap)
	def apply[T <: Data](eltMap: ListMap[String, T]): RecordMap[T] = new RecordMap(eltMap)

[jackkoenig]

Are the type parameters needed here? Does the following now just work?

new RecordMap(ListMap(elements:_*))

[mwachs5]

it probably just works. will try

[jackkoenig]

Question about a comment, I also think you should apply the remaining 2 suggestions but functionally this looks good to me.