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

Fix ModuleChoice under D/I #4569

Merged
merged 4 commits into from
Dec 18, 2024
Merged

Fix ModuleChoice under D/I #4569

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a992b75
Use FileCheck in ModuleChoice tests; add a test
davidbiancolin Dec 18, 2024
826adbf
DRY out module definitions in ModuleChoiceSpec
davidbiancolin Dec 18, 2024
ac72698
Add a ModuleChoice test using Definition
davidbiancolin Dec 18, 2024
5be7f96
Ensure Groups are propagated to parent builder under D/I
davidbiancolin Dec 18, 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
1 change: 1 addition & 0 deletions core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ object Definition extends SourceInfoDoc {
Builder.components ++= ir.components
Builder.annotations ++= ir.annotations: @nowarn // this will go away when firrtl is merged
Builder.layers ++= dynamicContext.layers
Builder.options ++= dynamicContext.options
module._circuit = Builder.currentModule
module.toDefinition
}
Expand Down
132 changes: 70 additions & 62 deletions src/test/scala/chiselTests/ModuleChoiceSpec.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package chiselTests
import chisel3._
import chisel3.choice.{Case, Group, ModuleChoice}
import chiselTests.{ChiselFlatSpec, MatchesAndOmits, Utils}
import chisel3.experimental.hierarchy.Definition
import _root_.circt.stage.ChiselStage

object Platform extends Group {
Expand All @@ -17,54 +18,69 @@ class TargetIO(width: Int) extends Bundle {
val out = UInt(width.W)
}

class FPGATarget extends FixedIOExtModule[TargetIO](new TargetIO(8))
class FPGATarget extends FixedIORawModule[TargetIO](new TargetIO(8)) {
io.out := io.in
}

class ASICTarget extends FixedIOExtModule[TargetIO](new TargetIO(8))

class VerifTarget extends FixedIORawModule[TargetIO](new TargetIO(8))
class VerifTarget extends FixedIORawModule[TargetIO](new TargetIO(8)) {
io.out := io.in
}

class ModuleChoiceSpec extends ChiselFlatSpec with Utils with MatchesAndOmits {
it should "emit options and cases" in {
class ModuleWithChoice extends Module {
val out = IO(UInt(8.W))
class ModuleWithChoice[T <: Data](
default: => FixedIOBaseModule[T]
)(alternateImpls: Seq[(Case, () => FixedIOBaseModule[T])])
extends Module {
val inst = ModuleChoice(default)(alternateImpls)
val io = IO(inst.cloneType)
io <> inst
}

val inst =
ModuleChoice(new VerifTarget)(Seq(Platform.FPGA -> new FPGATarget, Platform.ASIC -> new ASICTarget))
class ModuleChoiceSpec extends ChiselFlatSpec with Utils with FileCheck {
it should "emit options and cases" in {
class ModuleWithValidChoices
extends ModuleWithChoice(new VerifTarget)(Seq(Platform.FPGA -> new FPGATarget, Platform.ASIC -> new ASICTarget))

generateFirrtlAndFileCheck(new ModuleWithValidChoices)(
"""|CHECK: option Platform :
|CHECK-NEXT: FPGA
|CHECK-NEXT: ASIC
|CHECK: instchoice inst of VerifTarget, Platform :
|CHECK-NEXT: FPGA => FPGATarget
|CHECK-NEXT: ASIC => ASICTarget""".stripMargin
)
}

inst.in := 42.U(8.W)
out := inst.out
it should "emit options and cases for Modules including definitions" in {
class ModuleWithValidChoices
extends ModuleWithChoice(new VerifTarget)(Seq(Platform.FPGA -> new FPGATarget, Platform.ASIC -> new ASICTarget))
class TopWithDefinition extends Module {
val definitionWithChoice = Definition(new ModuleWithValidChoices)
}

val chirrtl = ChiselStage.emitCHIRRTL(new ModuleWithChoice, Array("--full-stacktrace"))

info("CHIRRTL emission looks correct")
matchesAndOmits(chirrtl)(
"option Platform :",
"FPGA",
"ASIC",
"instchoice inst of VerifTarget, Platform :",
"FPGA => FPGATarget",
"ASIC => ASICTarget"
)()
generateFirrtlAndFileCheck(new TopWithDefinition)(
"""|CHECK: option Platform :
|CHECK-NEXT: FPGA
|CHECK-NEXT: ASIC
|CHECK: instchoice inst of VerifTarget, Platform :
|CHECK-NEXT: FPGA => FPGATarget
|CHECK-NEXT: ASIC => ASICTarget""".stripMargin
)
}

it should "require that all cases are part of the same option" in {

class MixedOptions extends Module {
object Performance extends Group {
object Fast extends Case
object Small extends Case
}

val out = IO(UInt(8.W))

val inst =
ModuleChoice(new VerifTarget)(Seq(Platform.FPGA -> new FPGATarget, Performance.Fast -> new ASICTarget))

inst.in := 42.U(8.W)
out := inst.out
object Performance extends Group {
object Fast extends Case
object Small extends Case
}

class MixedOptions
extends ModuleWithChoice(new VerifTarget)(
Seq(Platform.FPGA -> new FPGATarget, Performance.Fast -> new ASICTarget)
)

intercept[IllegalArgumentException] { ChiselStage.emitCHIRRTL(new MixedOptions) }.getMessage() should include(
"cannot mix choices from different groups: Platform, Performance"
)
Expand All @@ -73,33 +89,33 @@ class ModuleChoiceSpec extends ChiselFlatSpec with Utils with MatchesAndOmits {

it should "require that at least one alternative is present" in {

class MixedOptions extends Module {
val out = IO(UInt(8.W))

val inst =
ModuleChoice(new VerifTarget)(Seq())
class NoAlternatives extends ModuleWithChoice(new VerifTarget)(Seq())

inst.in := 42.U(8.W)
out := inst.out
}

intercept[IllegalArgumentException] { ChiselStage.emitCHIRRTL(new MixedOptions) }.getMessage() should include(
intercept[IllegalArgumentException] { ChiselStage.emitCHIRRTL(new NoAlternatives) }.getMessage() should include(
"at least one alternative must be specified"
)

}

it should "require that all cases are distinct" in {
it should "allow a subset of options to be provided" in {

class SubsetOptions extends ModuleWithChoice(new VerifTarget)(Seq(Platform.FPGA -> new FPGATarget))

class MixedOptions extends Module {
val out = IO(UInt(8.W))
// Note, because of a quirk in how [[Case]]s are registered, only those referenced
// in the Module here are going to be captured. This will be fixed in a forthcoming PR
// that implements an [[addLayer]] like feature for [[Group]]s
generateFirrtlAndFileCheck(new SubsetOptions)(
"""|CHECK: option Platform :
|CHECK-NEXT: FPGA
|CHECK: instchoice inst of VerifTarget, Platform :
|CHECK-NEXT: FPGA => FPGATarget""".stripMargin
)
}

val inst =
ModuleChoice(new VerifTarget)(Seq(Platform.FPGA -> new FPGATarget, Platform.FPGA -> new ASICTarget))
it should "require that all cases are distinct" in {

inst.in := 42.U(8.W)
out := inst.out
}
class MixedOptions
extends ModuleWithChoice(new VerifTarget)(Seq(Platform.FPGA -> new FPGATarget, Platform.FPGA -> new ASICTarget))

intercept[IllegalArgumentException] { ChiselStage.emitCHIRRTL(new MixedOptions) }.getMessage() should include(
"duplicate case 'FPGA'"
Expand All @@ -109,17 +125,9 @@ class ModuleChoiceSpec extends ChiselFlatSpec with Utils with MatchesAndOmits {

it should "require that all IO bundles are type equivalent" in {

class MixedIO extends Module {
val out = IO(UInt(8.W))
class Target16 extends FixedIOExtModule[TargetIO](new TargetIO(16))

class Target16 extends FixedIOExtModule[TargetIO](new TargetIO(16))

val inst =
ModuleChoice(new VerifTarget)(Seq(Platform.FPGA -> new Target16))

inst.in := 42.U(8.W)
out := inst.out
}
class MixedIO extends ModuleWithChoice(new VerifTarget)(Seq(Platform.FPGA -> new Target16))

intercept[ChiselException] { ChiselStage.emitCHIRRTL(new MixedIO, Array("--throw-on-first-error")) }
.getMessage() should include(
Expand Down
Loading