From 00705702bfc201ee95d5b8f33d4d1e46eb799cdc Mon Sep 17 00:00:00 2001 From: David Biancolin Date: Wed, 18 Dec 2024 08:46:15 -0800 Subject: [PATCH] Fix ModuleChoice under D/I (#4569) * 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 --- .../hierarchy/core/Definition.scala | 1 + .../scala/chiselTests/ModuleChoiceSpec.scala | 132 ++++++++++-------- 2 files changed, 71 insertions(+), 62 deletions(-) diff --git a/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala b/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala index 7f584963877..02d91873096 100644 --- a/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala +++ b/core/src/main/scala/chisel3/experimental/hierarchy/core/Definition.scala @@ -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 } diff --git a/src/test/scala/chiselTests/ModuleChoiceSpec.scala b/src/test/scala/chiselTests/ModuleChoiceSpec.scala index 86da88ede1a..853391292b9 100644 --- a/src/test/scala/chiselTests/ModuleChoiceSpec.scala +++ b/src/test/scala/chiselTests/ModuleChoiceSpec.scala @@ -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 { @@ -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" ) @@ -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'" @@ -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(