PRCI: use new RecordMap API to name signals

[mwachs5]

Related issue: This builds on top of #2486 and will be rebased on top of it as that API changes.

This demonstrates the use of the new RecordListMap. API to get better names for clock group signals

In the comments I'll include a diff before and after this change.

Type of change:feature request

Impact: API modification

(Generated RTL will have different output)

Development Phase: implementation

Release Notes

I/O and internal signals in the clock related modules (ClockSource, ClockGroup, etc) will have more meaninguful I/O names vs simply indexed names.

[mwachs5]

@hcook I am right that only the sink has a name, right? there is no source so I can't do something like "source_to_sink" right?

[hcook]

Correct, though such functionality would be an argument for adding such a parameter. The source-side parameters are very underdeveloped. We could also think about adding the NodePath functionality that some of the other protocols have, then lieu of a parameterized name you might be able to get the ValName of the source node or the module name of it's parent.

[hcook]

But even just knowing the sink is useful IMO

[mwachs5]

attaching the diff for DefaultConfig:
diff.txt

For example, old:

module ClockGroupAggregator_1( // @[:freechips.rocketchip.system.DefaultConfig.fir@28996.2]
  input   auto_in_member_0_clock, // @[:freechips.rocketchip.system.DefaultConfig.fir@28997.4]
  input   auto_in_member_0_reset, // @[:freechips.rocketchip.system.DefaultConfig.fir@28997.4]
  output  auto_out_member_0_clock, // @[:freechips.rocketchip.system.DefaultConfig.fir@28997.4]
  output  auto_out_member_0_reset // @[:freechips.rocketchip.system.DefaultConfig.fir@28997.4]
);

new:

module ClockGroupAggregator( // @[:freechips.rocketchip.system.DefaultConfig.fir@44.2]
  input   auto_in_member_subsystem_sbus_0_clock, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  input   auto_in_member_subsystem_sbus_0_reset, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  output  auto_out_0_member_subsystem_sbus_0_clock, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  output  auto_out_0_member_subsystem_sbus_0_reset // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
);

[mwachs5]

old:

module ClockGroupAggregator( // @[:freechips.rocketchip.system.DefaultConfig.fir@44.2]
  input   auto_in_member_5_clock, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  input   auto_in_member_5_reset, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  input   auto_in_member_4_clock, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  input   auto_in_member_4_reset, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  input   auto_in_member_3_clock, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  input   auto_in_member_3_reset, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  input   auto_in_member_2_clock, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  input   auto_in_member_2_reset, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  input   auto_in_member_1_clock, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  input   auto_in_member_1_reset, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  input   auto_in_member_0_clock, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  input   auto_in_member_0_reset, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  output  auto_out_3_member_1_clock, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  output  auto_out_3_member_1_reset, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  output  auto_out_3_member_0_clock, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  output  auto_out_3_member_0_reset, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  output  auto_out_2_member_0_clock, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  output  auto_out_2_member_0_reset, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  output  auto_out_1_member_1_clock, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  output  auto_out_1_member_1_reset, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  output  auto_out_1_member_0_clock, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  output  auto_out_1_member_0_reset, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  output  auto_out_0_member_0_clock, // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
  output  auto_out_0_member_0_reset // @[:freechips.rocketchip.system.DefaultConfig.fir@45.4]
);

new:

...? Actually not sure what happened to this one in the new world.

[mwachs5]

so maybe I screwed something up here...

[mwachs5]

this is basically a rewrite of makeIOs from diplomacy.Nodes, which assumes Heterogeneous Bag

[hcook]

Yeah in a follow up PR we should just entirely replace that use of HeterogeneousBag with RecordListMap, I think. Will require sed/updating a moderate amount of boilerplate closed source code to change arguments like "io: HeterogeneousBag" to "io: Record" though

[mwachs5]

This is currently giving me a compile error (just running make verilog) that I don't understand (and also seems weirdly formatted):

info] [0.001] Elaborating design...
[error] java.lang.ClassCastException: freechips.rocketchip.prci.ClockBundle cannot be cast to scala.runtime.Nothing$
[error] 	...
[error] ocketchiat freechips.rocketchip.util.RecordMap.$anonfun$elements$1(RecordMap.scala:22)
[error] 	at scala.collection.MapLike$MappedValues.$anonfun$foreach$3(MapLike.scala:256)
[error] 	at scala.collection.TraversableLike$WithFilter.$anonfun$foreach$1(TraversableLike.scala:877)

[mwachs5]

Here's the current example of what this creates:

I am still a little confused why we end up with only one output instead of 6 in the SimpleClockGroupSource, I think I am checking for duplicates in the wrong place in RecordMap (does a ListMap already not allow duplicates?)

[mwachs5]

Just as an update I'm still very weirded out by the before-and-after diff. If I look at the fir that comes out, the diff looks as I expect: all the same wires and I/Os are there, they just have better names ("member.sbus_0.clock" vs "member.0.clock" and so on). But in the verilog most of the signals are removed including entire modules. I am going to do a run with the split-trace on to see when they diverge.

[mwachs5]

I think I figured out what was wrong /different with this.

In the original, this line does something:
https://github.com/chipsalliance/rocket-chip/blob/master/src/main/scala/diplomacy/LazyModule.scala#L292

In this new version, that is tied off to 1'b0:

Which may be why things get constant-propped away. Still looking at the FIRRTL intermediate to see where things diverge

[mwachs5]

im not sure why this PR is closed but also says "merged and closed". Something is weird in github-land. Anyway, I have a suspicion what is wrong here... somewhere I am combining HeterogeneousBags of ClockGroupBundles whose names don't match up. I think the auto-bundle-zipping of diplomacy (:=) is then not matching them up. In other words, somewhere I am trying to attach a HeterogenousBag[ClockGroupBundle] := HeterogeneousBag[ClockGroupBundle] and since the two ClockGroupBundles have different RecordMap parameters, the signals don't just magically get zipped together. Need to figure out where this is happening and fix it.