Skip to content

Commit

Permalink
Fix ModuleChoice under D/I (#4569)
Browse files Browse the repository at this point in the history 
* Use FileCheck in ModuleChoice tests; add a test
* DRY out module definitions in ModuleChoiceSpec
* Add a ModuleChoice test using Definition
* Ensure Groups are propagated to parent builder under D/I

(cherry picked from commit 0070570)

# Conflicts:
#	core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala
  • Loading branch information
@davidbiancolin @mergify
davidbiancolin authored and mergify[bot] committed Dec 18, 2024
1 parent 038e38c commit ed549ac
Show file tree
Hide file tree
Showing 2 changed files with 75 additions and 62 deletions.
5 changes: 5 additions & 0 deletions core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,11 @@ object Definition extends SourceInfoDoc {
val (ir, module) = Builder.build(Module(proto), dynamicContext, false)
Builder.components ++= ir.components
Builder.annotations ++= ir.annotations: @nowarn // this will go away when firrtl is merged
<<<<<<< HEAD
=======
Builder.layers ++= dynamicContext.layers
Builder.options ++= dynamicContext.options
>>>>>>> 00705702 (Fix ModuleChoice under D/I (#4569))
module._circuit = Builder.currentModule
module.toDefinition
}
Expand Down
132 changes: 70 additions & 62 deletions src/test/scala/chiselTests/ModuleChoiceSpec.scala
View file
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

0 comments on commit ed549ac

Please sign in to comment.