From a8d7b9a2a118ac3763935664309c031b91d48ca0 Mon Sep 17 00:00:00 2001 From: Schuyler Eldridge Date: Fri, 28 Aug 2020 12:09:43 -0400 Subject: Deprecate CompilerAnnotation (#1870) * CompilerAnnotation$ emits RunFirrtlTransform Change the CompilerAnnotation object to emit RunFirrtlTransformAnnotations containing the associated emitter. This requires a fix in the Driver compatibility layer to know how to enable one-file-per module emission if either a CompilerAnnotation or a RunFirrtlTransformAnnotation(_: Emitter) is present. Signed-off-by: Schuyler Eldridge * Add ConvertCompilerAnnotation phase Add a phase, ConvertCompilerAnnotation, that converts a CompilerAnnotation to a RunFirrtlTransformAnnotation. This provides a warning to the user if this path is taken. Signed-off-by: Schuyler Eldridge * Add test of ConvertCompilerAnnotation Signed-off-by: Schuyler Eldridge * Deprecate CompilerAnnotation$, move helper methods Deprecate the CompilerAnnotation companion object and move it's private utility inside the RunFirrtlTransformAnnotation companion object. Signed-off-by: Schuyler Eldridge * Make ConvertCompilerAnnotations private[firrtl] Make this phase private to avoid adding a deprecation warning. Also, remove an unused string value. Signed-off-by: Schuyler Eldridge * Fix incorrect string in test Signed-off-by: Schuyler Eldridge * Add test that '-X verilog', no emitter yields file Signed-off-by: Schuyler Eldridge --- .../tests/ConvertCompilerAnnotationsSpec.scala | 40 ++++++++++++++++++++++ src/test/scala/firrtl/testutils/FirrtlSpec.scala | 2 +- .../scala/firrtlTests/stage/FirrtlMainSpec.scala | 6 ++++ .../firrtlTests/stage/phases/AddDefaultsSpec.scala | 10 ++++-- .../firrtlTests/stage/phases/ChecksSpec.scala | 14 ++++---- 5 files changed, 61 insertions(+), 11 deletions(-) create mode 100644 src/test/scala/firrtl/stage/phases/tests/ConvertCompilerAnnotationsSpec.scala (limited to 'src/test') diff --git a/src/test/scala/firrtl/stage/phases/tests/ConvertCompilerAnnotationsSpec.scala b/src/test/scala/firrtl/stage/phases/tests/ConvertCompilerAnnotationsSpec.scala new file mode 100644 index 00000000..acd37bf5 --- /dev/null +++ b/src/test/scala/firrtl/stage/phases/tests/ConvertCompilerAnnotationsSpec.scala @@ -0,0 +1,40 @@ +// See LICENSE for license details. + +package firrtl.stage.phases.tests + +import firrtl.{HighFirrtlCompiler, HighFirrtlEmitter, LowFirrtlCompiler} +import firrtl.options.{Dependency, OptionsException} +import firrtl.stage.{CompilerAnnotation, RunFirrtlTransformAnnotation} +import firrtl.stage.phases.ConvertCompilerAnnotations + +import org.scalatest.flatspec.AnyFlatSpec +import org.scalatest.matchers.should.Matchers + +class ConvertCompilerAnnotationsSpec extends AnyFlatSpec with Matchers { + + class Fixture { + val phase = new ConvertCompilerAnnotations + } + + behavior.of(classOf[ConvertCompilerAnnotations].getName) + + it should "convert a deprecated CompilerAnnotation to a RunFirrtlTransformAnnotation" in new Fixture { + val annotations = Seq(CompilerAnnotation(new HighFirrtlCompiler)) + phase + .transform(annotations) + .map { + case RunFirrtlTransformAnnotation(a) => Dependency.fromTransform(a) + } + .toSeq should be(Seq(Dependency[HighFirrtlEmitter])) + } + + it should "throw an exception if multiple CompilerAnnotations are specified" in new Fixture { + val annotations = Seq( + CompilerAnnotation(new HighFirrtlCompiler), + CompilerAnnotation(new LowFirrtlCompiler) + ) + intercept[OptionsException] { + phase.transform(annotations) + }.getMessage should include("Zero or one CompilerAnnotation may be specified") + } +} diff --git a/src/test/scala/firrtl/testutils/FirrtlSpec.scala b/src/test/scala/firrtl/testutils/FirrtlSpec.scala index a0c41085..9ee5fdaf 100644 --- a/src/test/scala/firrtl/testutils/FirrtlSpec.scala +++ b/src/test/scala/firrtl/testutils/FirrtlSpec.scala @@ -101,7 +101,7 @@ trait FirrtlRunners extends BackendCompilationUtilities { InfoModeAnnotation("ignore") +: RenameTopAnnotation(topName) +: stage.FirrtlCircuitAnnotation(circuit) +: - stage.CompilerAnnotation("mverilog") +: + stage.RunFirrtlTransformAnnotation.stringToEmitter("mverilog") +: stage.OutputFileAnnotation(topName) +: toAnnos(baseTransforms) } diff --git a/src/test/scala/firrtlTests/stage/FirrtlMainSpec.scala b/src/test/scala/firrtlTests/stage/FirrtlMainSpec.scala index 9274bac6..ee942d85 100644 --- a/src/test/scala/firrtlTests/stage/FirrtlMainSpec.scala +++ b/src/test/scala/firrtlTests/stage/FirrtlMainSpec.scala @@ -286,6 +286,12 @@ class FirrtlMainSpec args = Array("-X", "sverilog", "-E", "sverilog", "-o", "Foo"), stdout = defaultStdOut, files = Seq("Foo.sv") + ), + /* Test that an output is generated if no emitter is specified */ + FirrtlMainTest( + args = Array("-X", "verilog", "-o", "Foo"), + stdout = defaultStdOut, + files = Seq("Foo.v") ) ) .foreach(runStageExpectFiles) diff --git a/src/test/scala/firrtlTests/stage/phases/AddDefaultsSpec.scala b/src/test/scala/firrtlTests/stage/phases/AddDefaultsSpec.scala index 686c42ad..eca7b706 100644 --- a/src/test/scala/firrtlTests/stage/phases/AddDefaultsSpec.scala +++ b/src/test/scala/firrtlTests/stage/phases/AddDefaultsSpec.scala @@ -2,11 +2,11 @@ package firrtlTests.stage.phases -import firrtl.NoneCompiler +import firrtl.ChirrtlEmitter import firrtl.annotations.Annotation import firrtl.stage.phases.AddDefaults import firrtl.transforms.BlackBoxTargetDirAnno -import firrtl.stage.{CompilerAnnotation, InfoModeAnnotation, RunFirrtlTransformAnnotation} +import firrtl.stage.{InfoModeAnnotation, RunFirrtlTransformAnnotation} import firrtl.options.{Dependency, Phase, TargetDirAnnotation} import org.scalatest.flatspec.AnyFlatSpec import org.scalatest.matchers.should.Matchers @@ -32,7 +32,11 @@ class AddDefaultsSpec extends AnyFlatSpec with Matchers { } it should "not overwrite existing annotations" in new Fixture { - val input = Seq(BlackBoxTargetDirAnno("foo"), CompilerAnnotation(new NoneCompiler()), InfoModeAnnotation("ignore")) + val input = Seq( + BlackBoxTargetDirAnno("foo"), + RunFirrtlTransformAnnotation(new ChirrtlEmitter), + InfoModeAnnotation("ignore") + ) phase.transform(input).toSeq should be(input) } diff --git a/src/test/scala/firrtlTests/stage/phases/ChecksSpec.scala b/src/test/scala/firrtlTests/stage/phases/ChecksSpec.scala index 65516da5..6c0335b9 100644 --- a/src/test/scala/firrtlTests/stage/phases/ChecksSpec.scala +++ b/src/test/scala/firrtlTests/stage/phases/ChecksSpec.scala @@ -4,7 +4,7 @@ package firrtlTests.stage.phases import firrtl.stage._ -import firrtl.{AnnotationSeq, ChirrtlEmitter, EmitAllModulesAnnotation, NoneCompiler} +import firrtl.{AnnotationSeq, ChirrtlEmitter, EmitAllModulesAnnotation} import firrtl.options.{OptionsException, OutputAnnotationFileAnnotation, Phase} import firrtl.stage.phases.Checks import org.scalatest.flatspec.AnyFlatSpec @@ -18,7 +18,7 @@ class ChecksSpec extends AnyFlatSpec with Matchers { val outputFile = OutputFileAnnotation("bar") val emitAllModules = EmitAllModulesAnnotation(classOf[ChirrtlEmitter]) val outputAnnotationFile = OutputAnnotationFileAnnotation("baz") - val goodCompiler = CompilerAnnotation(new NoneCompiler()) + val goodCompiler = RunFirrtlTransformAnnotation(new ChirrtlEmitter) val infoMode = InfoModeAnnotation("ignore") val min = Seq(inputFile, goodCompiler, infoMode) @@ -54,15 +54,15 @@ class ChecksSpec extends AnyFlatSpec with Matchers { checkExceptionMessage(phase, in, "No more than one output file can be specified") } - it should "enforce exactly one compiler" in new Fixture { + it should "enforce one or more compilers (at this point these are emitters)" in new Fixture { info("0 compilers should throw an exception") val inZero = Seq(inputFile, infoMode) - checkExceptionMessage(phase, inZero, "Exactly one compiler must be specified, but none found") + checkExceptionMessage(phase, inZero, "At least one compiler must be specified") - info("2 compilers should throw an exception") - val c = goodCompiler.compiler + info("2 compilers should not throw an exception") + val c = goodCompiler val inTwo = min :+ goodCompiler - checkExceptionMessage(phase, inTwo, s"Exactly one compiler must be specified, but found '$c, $c'") + phase.transform(inTwo) } it should "validate info mode names" in new Fixture { -- cgit v1.2.3