[choice] Adds a means to declare a choice group without use

[davidbiancolin]

Per @seldridge's suggestion here I add analog of addLayer for ModuleChoice called addGroup. This permits defining a group in a circuit without necessarily using that group in a circuit, allowing for commonly reused top-level modules to always have these choices defined regardless of what is instantiated in their module hierarchies.

Short of reworking the group API a bit to more explicitly register the Cases in the Group at construction time i couldn't figure out a means to do what i wanted, so i resorted to using Scala reflection. If there's a smarter way to do this i'm all ears.

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 request a desired merge strategy?
• Did you add text to be included in the Release Notes for this change?

Type of Improvement

• Feature (or new API)

Desired Merge Strategy

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

Release Notes

Introduce choice.addGroup which enables declaration of a choice.Group without its use (akin to layer.addLayer)

Reviewer Checklist (only modified by reviewer)

• Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
• Did you mark the proper milestone (Bug fix: 3.6.x, 5.x, or 6.x depending on impact, API modification or big change: 7.0)?
• Did you review?
• Did you check whether all relevant Contributor checkboxes have been checked?
• Did you do one of the following when ready to merge:
  • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
  • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

[seldridge]

The API of addGroup looks great.

One suggestion on how to avoid the reflection and a nit in the test to use FileCheck.

[seldridge]

Nit: Array("--full-stacktrace") can be dropped.

[seldridge]

This is clever reflection. 👍 However, it may be better to just make this information more easily accessible. Two changes would avoid the TypeTag (which is a Scala3 migration annoyance, I think) and any work in this function:

1. Add a Builder member for storing groups and not just options.
2. Add mutable.LinkedHashSet to Group that can be used to directly look up what Cases it supports. Then modify Case to insert into it.

[davidbiancolin]

I started doing a (2) but wasn't sure when/how to finalize the set short of doing something either janky or switching the API in someway to use a different pattern.

But (1) seems really easy to do; the existing struct can probably just become a HashMap and we wouldn't need to reconstruct the the groups later.

[davidbiancolin]

Ok i got rid of the type tag but i can't figure out how to avoid it altogether unless we change the API for defining these cases (so that they are not lazy initialized).

Alternatively, we could change the addGroup API to to instead take a one or more cases. For example addGroupHint(c: Case) would suffice to workaround my problem, but it seems rather hacky to not register the whole group and breaks the direct addLayer analogy.

However, I doubt breaking the Group API here is a big deal since no one should be using this? I'd change the Group` implementation to use a more factory like pattern that would force registration on the definition of the objects.

LMK

[seldridge]

Yeah, I think the laziness of the cases seems wrong. Laziness of groups seems fine. (You only get a group if you use it, but you get all of its cases.)

This would be a change to have the Builder register groups, have cases register that they are children of groups, and then pushing that change through to the converter.

However, all of this is independent of any change to the API. Given that, I will go ahead and approve this.

[jackkoenig]

I'm not sure if this reflection works in Scala 3 and as we are actively trying to get Scala 3 compilation into CI, I think we need to address it sooner rather than later. Can we do something similar to how Layers get the parents with an implicit rather than reflecting on the Group?

[davidbiancolin]

Well, can i break the existing API? If so this is pretty straight forward. Otherwise i might need one of you guys (i.e., someone with more Scala foo than i) to do it. The crux of it is that the Cases are lazily constructed so something has to reference them to bind them to the group. If you're coming into the office today maybe we can chat about it?

[seldridge]

You can break it as much as you want. Just don't backport it to 6.x. This feature has no users and you won't be disturbing anyone.

[seldridge]

LGTM

The internals of groups need to be reworked. That can be done in a follow-on (or backlogged for someone else to do).

[davidbiancolin]

Also there seems to be some consistency referring to groups, cases and options in the code. Do you want me to go through and update the code and docs to match either the structures in here, or the ABI docs?

[jackkoenig]

@davidbiancolin and I discussed this offline

Unfortunately just having the Group as an implicit to the Case does not initialize the Case object, so to maintain the current API, the only way I can see to do it is with such reflection. I've advised David on how to split the reflection out into src/main/scala-2.

We discussed an alternative proposal though to switch to the style of ChiselEnum, basically instead of

object Platform extends Group {
  object FPGA extends Case
  object ASIC extends Case
}

You would write:

object Platform extends Group {
  val FPGA = Case
  val ASIC = Case
  val Special = Case("SpecialPlatform") // can manually pass the String too
}

This requires a simple macro to grab the val name a la ChiselEnum¹, or we could just upstream ValName and do it that way². I think this has 3 advantages:

1. It makes it impossible to accidentally nest a Case instead of another Case which isn't valid.
2. It's more consistent with enumeration style rather than layer-style (which does allow nesting).
3. I think compile-time reflection (aka macros) is almost always preferable to runtime-reflection. It's more transparent.

What do you think @seldridge?

Footnotes

1. https://github.com/chipsalliance/chisel/blob/4232cae4189abcebf1a7e244cb3875d85083e035/core/src/main/scala-2/chisel3/ChiselEnum.scala#L34 ↩

2. https://github.com/chipsalliance/rocket-chip/blob/2fe6bb5e9c23360cac8297bb26512e4ca614f67b/macros/src/main/scala/ValName.scala#L8 ↩

[seldridge]

Everything in this API can be broken as there are no users. I wouldn't worry about that. I would prefer to not introduce any new macros.

Why would something simple like having the Case insert themselves inside a mutable set inside the Group not work? This is what I've been in favor of even if it means the API changes.

I can see a potential reason to not require the nesting and to keep the implicit, though I'm not sure if it would come up. If we have builtin groups and cases defined, then we may want the ability to add new cases to an existing group. I do expect that we will eventually have built-in groups that memories use. It is possible that we want users to be able to add cases to these groups. (Alternatively, this could be just done with a generic blackbox.)

I do agree that these are trying to describe an enum.

[davidbiancolin]

Why would something simple like having the Case insert themselves inside a mutable set inside the Group not work?

The crux of it is Case cannot insert itself into a Group unless something references that Case, since they are objects and so lazily constructed. So either we have the user create that reference in the definition of group, or use a macro or reflection induce that reference without requiring the user to do so.

The prior could look as follows:

object Platform extends Group {
  object FPGA extends Case
  object ASIC extends Case
  val cases = Seq(FPGA, ASIC)
}

But this seems like a terrible design. Alternatively, one could make case a factory:

object Platform extends Group {
  val ASIC =  Case("ASIC")
  val FPGA = Case("FPGA")
}

Then you have the doubly define names that lead one towards using a macro as done in other libraries to auto-name the cases...

object Platform extends Group {
  val ASIC, FPGA =  Case
}

[adkian-sifive]

scala.reflect._ doesn't cross-compile with Scala 3, it needs to be put in core/src/main/scala-2/chisel3/