-
Notifications
You must be signed in to change notification settings - Fork 615
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.
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
Add new annotation for Chisel Circuit serialization #1580
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,34 @@ | ||||||
// See LICENSE for license details. | ||||||
|
||||||
package chisel3.stage.phases | ||||||
|
||||||
import firrtl.AnnotationSeq | ||||||
import firrtl.options.{Dependency, Phase} | ||||||
import firrtl.options.Viewer.view | ||||||
|
||||||
import chisel3.stage._ | ||||||
import chisel3.stage.CircuitSerializationAnnotation._ | ||||||
import chisel3.internal.ChiselException | ||||||
|
||||||
/** Adds [[stage.CircuitSerializationAnnotation]]s based on [[ChiselOutputFileAnnotation]] | ||||||
*/ | ||||||
class AddSerializationAnnotations extends Phase { | ||||||
|
||||||
override def prerequisites = Seq(Dependency[Elaborate], Dependency[AddImplicitOutputFile]) | ||||||
override def optionalPrerequisites = Seq.empty | ||||||
override def optionalPrerequisiteOf = Seq.empty | ||||||
override def invalidates(a: Phase) = false | ||||||
|
||||||
def transform(annotations: AnnotationSeq): AnnotationSeq = { | ||||||
val chiselOptions = view[ChiselOptions](annotations) | ||||||
val circuit = chiselOptions.chiselCircuit.getOrElse { | ||||||
throw new ChiselException(s"Unable to locate the elaborated circuit, did ${classOf[Elaborate].getName} run correctly") | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm of two minds on whether or not this should throw an error. Some phases, like By this logic, the error message here is a bit odd as it isn't that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To clarify: this is fine as it stands. I would just like some eventual unification in how we handle this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I see your point. I wasn't sure what to do here so I just decided to throw an error, but I'm happy to change this behavior |
||||||
} | ||||||
val baseFilename = chiselOptions.outputFile.getOrElse(circuit.name) | ||||||
|
||||||
val (filename, format) = | ||||||
if (baseFilename.endsWith(".pb")) (baseFilename.stripSuffix(".pb"), ProtoBufFileFormat) | ||||||
else (baseFilename.stripSuffix(".fir"), FirrtlFileFormat) | ||||||
CircuitSerializationAnnotation(circuit, filename, format) +: annotations | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,9 @@ import org.scalatest.GivenWhenThen | |
import org.scalatest.featurespec.AnyFeatureSpec | ||
import org.scalatest.matchers.should.Matchers | ||
|
||
import scala.io.Source | ||
import firrtl.Parser | ||
|
||
object ChiselMainSpec { | ||
|
||
/** A module that connects two different types together resulting in an elaboration error */ | ||
|
@@ -52,7 +55,7 @@ class ChiselMainSpec extends AnyFeatureSpec with GivenWhenThen with Matchers wit | |
} | ||
|
||
class TargetDirectoryFixture(dirName: String) { | ||
val dir = new File(s"test_run_dir/FirrtlStageSpec/$dirName") | ||
val dir = new File(s"test_run_dir/ChiselStageSpec/$dirName") | ||
val buildDir = new File(dir + "/build") | ||
dir.mkdirs() | ||
} | ||
|
@@ -63,7 +66,8 @@ class ChiselMainSpec extends AnyFeatureSpec with GivenWhenThen with Matchers wit | |
files: Seq[String] = Seq.empty, | ||
stdout: Option[String] = None, | ||
stderr: Option[String] = None, | ||
result: Int = 0) { | ||
result: Int = 0, | ||
fileChecks: Map[String, File => Unit] = Map.empty) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice addition. |
||
def testName: String = "args" + args.mkString("_") | ||
def argsString: String = args.mkString(" ") | ||
} | ||
|
@@ -117,6 +121,7 @@ class ChiselMainSpec extends AnyFeatureSpec with GivenWhenThen with Matchers wit | |
And(s"file '$f' should be emitted in the target directory") | ||
val out = new File(td.buildDir + s"/$f") | ||
out should (exist) | ||
p.fileChecks.get(f).map(_(out)) | ||
} | ||
} | ||
} | ||
|
@@ -148,6 +153,33 @@ class ChiselMainSpec extends AnyFeatureSpec with GivenWhenThen with Matchers wit | |
).foreach(runStageExpectFiles) | ||
} | ||
|
||
Feature("Specifying a custom output file") { | ||
runStageExpectFiles(ChiselMainTest( | ||
args = Array("--chisel-output-file", "Foo", "--no-run-firrtl"), | ||
generator = Some(classOf[SameTypesModule]), | ||
stdout = Some(""), | ||
files = Seq("Foo.fir"), | ||
fileChecks = Map( | ||
"Foo.fir" -> { file => | ||
And("It should be valid FIRRTL") | ||
Parser.parse(Source.fromFile(file).mkString) | ||
} | ||
) | ||
)) | ||
runStageExpectFiles(ChiselMainTest( | ||
args = Array("--chisel-output-file", "Foo.pb", "--no-run-firrtl"), | ||
generator = Some(classOf[SameTypesModule]), | ||
stdout = Some(""), | ||
files = Seq("Foo.pb"), | ||
fileChecks = Map( | ||
"Foo.pb" -> { file => | ||
And("It should be valid ProtoBuf") | ||
firrtl.proto.FromProto.fromFile(file.toString) | ||
} | ||
) | ||
)) | ||
} | ||
|
||
info("As an aspect writer") | ||
info("I write an aspect") | ||
Feature("Running aspects via the command line") { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
// See LICENSE for license details. | ||
|
||
package chiselTests.stage.phases | ||
|
||
|
||
import chisel3.RawModule | ||
import chisel3.stage.{ChiselGeneratorAnnotation, ChiselOutputFileAnnotation, CircuitSerializationAnnotation} | ||
import chisel3.stage.CircuitSerializationAnnotation._ | ||
import chisel3.stage.phases.{AddSerializationAnnotations, AddImplicitOutputFile, Elaborate} | ||
|
||
import firrtl.AnnotationSeq | ||
import firrtl.options.{Phase, PhaseManager, Dependency, TargetDirAnnotation} | ||
import firrtl.options.Viewer.view | ||
import org.scalatest.flatspec.AnyFlatSpec | ||
import org.scalatest.matchers.should.Matchers | ||
|
||
class AddSerializationAnnotationsSpec extends AnyFlatSpec with Matchers { | ||
|
||
class Foo extends RawModule { override val desiredName = "Foo" } | ||
|
||
class Fixture { | ||
val phase: Phase = new AddSerializationAnnotations | ||
val manager = new PhaseManager(Dependency[AddSerializationAnnotations] :: Nil) | ||
} | ||
|
||
behavior of classOf[AddSerializationAnnotations].toString | ||
|
||
it should "default to FirrtlFileFormat" in new Fixture { | ||
val annotations: AnnotationSeq = Seq( | ||
ChiselGeneratorAnnotation(() => new Foo), | ||
ChiselOutputFileAnnotation("Bar") ) | ||
|
||
manager | ||
.transform(annotations) | ||
.collect { case CircuitSerializationAnnotation(_, filename, format) => (filename, format) } | ||
.toSeq should be (Seq(("Bar", FirrtlFileFormat))) | ||
} | ||
|
||
it should "support ProtoBufFileFormat" in new Fixture { | ||
val annotations: AnnotationSeq = Seq( | ||
ChiselGeneratorAnnotation(() => new Foo), | ||
ChiselOutputFileAnnotation("Bar.pb") ) | ||
|
||
manager | ||
.transform(annotations) | ||
.collect { case CircuitSerializationAnnotation(_, filename, format) => (filename, format) } | ||
.toSeq should be (Seq(("Bar", ProtoBufFileFormat))) | ||
} | ||
|
||
it should "support explicitly asking for FirrtlFileFormat" in new Fixture { | ||
val annotations: AnnotationSeq = Seq( | ||
ChiselGeneratorAnnotation(() => new Foo), | ||
ChiselOutputFileAnnotation("Bar.pb.fir") ) | ||
|
||
manager | ||
.transform(annotations) | ||
.collect { case CircuitSerializationAnnotation(_, filename, format) => (filename, format) } | ||
.toSeq should be (Seq(("Bar.pb", FirrtlFileFormat))) | ||
} | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch.