Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7a2a62f
[choice] Adds a means to declare a choice group without use
davidbiancolin Dec 10, 2024
8dbb9b8
Update src/test/scala/chiselTests/ModuleChoiceSpec.scala
davidbiancolin Dec 10, 2024
a53063d
A slightly different implementation avoiding TypeTag
davidbiancolin Dec 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion core/src/main/scala/chisel3/choice/package.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,13 @@
package chisel3

import chisel3.experimental.{BaseModule, SourceInfo}
import chisel3.internal.Builder
import chisel3.util.simpleClassName

import scala.collection.mutable
import scala.reflect.runtime.universe._
import scala.reflect.runtime.{currentMirror => cm}
Comment on lines +10 to +11
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/** This package contains Chisel language definitions for describing configuration options and their accepted values.
*/
package object choice {
Expand All @@ -13,7 +18,7 @@ package object choice {
*
* @example
* {{{
* import chisel3.option.{Group, Case}
* import chisel3.choice.{Group, Case}
* object Platform extends Group {
* object FPGA extends Case
* object ASIC extends Case
Expand Down Expand Up @@ -47,4 +52,27 @@ package object choice {
*/
def ->[T](module: => T): (Case, () => T) = (this, () => module)
}

/** Registers all options in a group with the Builder.
* This lets Chisel know that this layer should be emitted into FIRRTL text.
*
* This API can be used to guarantee that a design will always have certain
* group defined. This is analagous in spirit to [[layer.addLayer]].
*/
def addGroup[T <: Group: TypeTag](group: T): Unit = {

val tpe = typeOf[T]
val classSymbol = tpe.typeSymbol.asClass
val classMirror = cm.reflectClass(classSymbol)

tpe.members.collect {
// Look only for inner objects. Note, this is not recursive.
case m: ModuleSymbol if m.isStatic =>
val instance = cm.reflectModule(m.asModule).instance
// Confirms the instance is a subtype of Case
if (cm.classSymbol(instance.getClass).toType <:< typeOf[Case]) {
Builder.options += instance.asInstanceOf[Case]
}
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
21 changes: 20 additions & 1 deletion src/test/scala/chiselTests/ModuleChoiceSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
package chiselTests

import chisel3._
import chisel3.choice.{Case, Group, ModuleChoice}
import chisel3.choice.{addGroup, Case, Group, ModuleChoice}
import chiselTests.{ChiselFlatSpec, MatchesAndOmits, Utils}
import _root_.circt.stage.ChiselStage

Expand Down Expand Up @@ -129,3 +129,22 @@ class ModuleChoiceSpec extends ChiselFlatSpec with Utils with MatchesAndOmits {
}

}
class AddGroupSpec extends ChiselFlatSpec with Utils with MatchesAndOmits {
it should "emit options for a registered group even if there are no consumers" in {
class ModuleWithoutChoice extends Module {
addGroup(Platform)
val out = IO(UInt(8.W))
val in = IO(UInt(8.W))
out := in
}

val chirrtl = ChiselStage.emitCHIRRTL(new ModuleWithoutChoice, Array("--full-stacktrace"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


info("CHIRRTL emission looks correct")
matchesAndOmits(chirrtl)(
"option Platform :",
"FPGA",
"ASIC"
)()
davidbiancolin marked this conversation as resolved.
Show resolved Hide resolved
}
}
Loading