From 9a3dcf761e40b7ac36f9c867d0a36692d4d74c0c Mon Sep 17 00:00:00 2001 From: Jack Koenig Date: Tue, 6 Apr 2021 11:02:20 -0700 Subject: Deprecate InlineCasts, add InlineAcrossCasts (#2146) To maintain binary compatibility, InlineAcrossCasts is just aliases to the now deprecated InlineCasts. We can make the binary incompatible change of renaming the class and object for 1.5. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>--- src/main/scala/firrtl/AddDescriptionNodes.scala | 2 +- .../firrtl/passes/VerilogModulusCleanup.scala | 2 +- src/main/scala/firrtl/passes/VerilogPrep.scala | 2 +- src/main/scala/firrtl/stage/Forms.scala | 2 +- .../scala/firrtl/transforms/FlattenRegUpdate.scala | 2 +- src/main/scala/firrtl/transforms/InlineCasts.scala | 13 +++- .../transforms/LegalizeClocksAndAsyncResets.scala | 2 +- .../transforms/RemoveKeywordCollisions.scala | 2 +- src/main/scala/firrtl/transforms/package.scala | 3 + .../scala/firrtlTests/InlineAcrossCastsSpec.scala | 77 ++++++++++++++++++++++ src/test/scala/firrtlTests/InlineCastsSpec.scala | 77 ---------------------- .../scala/firrtlTests/LoweringCompilersSpec.scala | 4 +- 12 files changed, 99 insertions(+), 89 deletions(-) create mode 100644 src/test/scala/firrtlTests/InlineAcrossCastsSpec.scala delete mode 100644 src/test/scala/firrtlTests/InlineCastsSpec.scala (limited to 'src') diff --git a/src/main/scala/firrtl/AddDescriptionNodes.scala b/src/main/scala/firrtl/AddDescriptionNodes.scala index 9424d4a7..123ae6e3 100644 --- a/src/main/scala/firrtl/AddDescriptionNodes.scala +++ b/src/main/scala/firrtl/AddDescriptionNodes.scala @@ -136,7 +136,7 @@ class AddDescriptionNodes extends Transform with DependencyAPIMigration { Dependency[firrtl.transforms.ReplaceTruncatingArithmetic], Dependency[firrtl.transforms.InlineBitExtractionsTransform], Dependency[firrtl.transforms.PropagatePresetAnnotations], - Dependency[firrtl.transforms.InlineCastsTransform], + Dependency[firrtl.transforms.InlineAcrossCastsTransform], Dependency[firrtl.transforms.LegalizeClocksTransform], Dependency[firrtl.transforms.FlattenRegUpdate], Dependency(passes.VerilogModulusCleanup), diff --git a/src/main/scala/firrtl/passes/VerilogModulusCleanup.scala b/src/main/scala/firrtl/passes/VerilogModulusCleanup.scala index baad2f4f..03dcf0a3 100644 --- a/src/main/scala/firrtl/passes/VerilogModulusCleanup.scala +++ b/src/main/scala/firrtl/passes/VerilogModulusCleanup.scala @@ -32,7 +32,7 @@ object VerilogModulusCleanup extends Pass { Dependency[firrtl.transforms.FixAddingNegativeLiterals], Dependency[firrtl.transforms.ReplaceTruncatingArithmetic], Dependency[firrtl.transforms.InlineBitExtractionsTransform], - Dependency[firrtl.transforms.InlineCastsTransform], + Dependency[firrtl.transforms.InlineAcrossCastsTransform], Dependency[firrtl.transforms.LegalizeClocksTransform], Dependency[firrtl.transforms.FlattenRegUpdate] ) diff --git a/src/main/scala/firrtl/passes/VerilogPrep.scala b/src/main/scala/firrtl/passes/VerilogPrep.scala index ed5db92e..9499889a 100644 --- a/src/main/scala/firrtl/passes/VerilogPrep.scala +++ b/src/main/scala/firrtl/passes/VerilogPrep.scala @@ -28,7 +28,7 @@ object VerilogPrep extends Pass { Dependency[firrtl.transforms.FixAddingNegativeLiterals], Dependency[firrtl.transforms.ReplaceTruncatingArithmetic], Dependency[firrtl.transforms.InlineBitExtractionsTransform], - Dependency[firrtl.transforms.InlineCastsTransform], + Dependency[firrtl.transforms.InlineAcrossCastsTransform], Dependency[firrtl.transforms.LegalizeClocksTransform], Dependency[firrtl.transforms.FlattenRegUpdate], Dependency(passes.VerilogModulusCleanup), diff --git a/src/main/scala/firrtl/stage/Forms.scala b/src/main/scala/firrtl/stage/Forms.scala index ab082151..4132f758 100644 --- a/src/main/scala/firrtl/stage/Forms.scala +++ b/src/main/scala/firrtl/stage/Forms.scala @@ -101,7 +101,7 @@ object Forms { Dependency[firrtl.transforms.FixAddingNegativeLiterals], Dependency[firrtl.transforms.ReplaceTruncatingArithmetic], Dependency[firrtl.transforms.InlineBitExtractionsTransform], - Dependency[firrtl.transforms.InlineCastsTransform], + Dependency[firrtl.transforms.InlineAcrossCastsTransform], Dependency[firrtl.transforms.LegalizeClocksTransform], Dependency[firrtl.transforms.FlattenRegUpdate], Dependency(passes.VerilogModulusCleanup), diff --git a/src/main/scala/firrtl/transforms/FlattenRegUpdate.scala b/src/main/scala/firrtl/transforms/FlattenRegUpdate.scala index 664ce1e6..3f497c91 100644 --- a/src/main/scala/firrtl/transforms/FlattenRegUpdate.scala +++ b/src/main/scala/firrtl/transforms/FlattenRegUpdate.scala @@ -170,7 +170,7 @@ class FlattenRegUpdate extends Transform with DependencyAPIMigration { Dependency[FixAddingNegativeLiterals], Dependency[ReplaceTruncatingArithmetic], Dependency[InlineBitExtractionsTransform], - Dependency[InlineCastsTransform], + Dependency[InlineAcrossCastsTransform], Dependency[LegalizeClocksTransform] ) diff --git a/src/main/scala/firrtl/transforms/InlineCasts.scala b/src/main/scala/firrtl/transforms/InlineCasts.scala index 761252c1..de54a326 100644 --- a/src/main/scala/firrtl/transforms/InlineCasts.scala +++ b/src/main/scala/firrtl/transforms/InlineCasts.scala @@ -10,6 +10,7 @@ import firrtl.options.Dependency import firrtl.Utils.{isBitExtract, isCast, NodeMap} +@deprecated("Replaced by InlineAcrossCastsTransform", "FIRRTL 1.4.3") object InlineCastsTransform { // Checks if an Expression is made up of only casts terminated by a Literal or Reference @@ -54,7 +55,7 @@ object InlineCastsTransform { rec(false)(expr) } - /** Inline casts in a Statement + /** Inline across casts in a statement * * @param netlist a '''mutable''' HashMap mapping references to [[firrtl.ir.DefNode DefNode]]s to their connected * [[firrtl.ir.Expression Expression]]s. This function '''will''' mutate @@ -71,11 +72,17 @@ object InlineCastsTransform { case other => other } - /** Replaces truncating arithmetic in a Module */ + /** Inline across casts in a module */ def onMod(mod: DefModule): DefModule = mod.map(onStmt(new NodeMap)) } -/** Inline nodes that are simple casts */ +/** Inline expressions into casts and inline casts into other expressions + * + * Because casts are no-ops in the emitted Verilog, this transform eliminates statements that + * simply contain a cast. It does so by greedily building larger expression trees that contain at + * most one expression that is neither a cast nor reference-like node. + */ +@deprecated("Replaced by InlineAcrossCastsTransform", "FIRRTL 1.4.3") class InlineCastsTransform extends Transform with DependencyAPIMigration { override def prerequisites = firrtl.stage.Forms.LowFormMinimumOptimized ++ diff --git a/src/main/scala/firrtl/transforms/LegalizeClocksAndAsyncResets.scala b/src/main/scala/firrtl/transforms/LegalizeClocksAndAsyncResets.scala index 5e3d276d..0765a2b1 100644 --- a/src/main/scala/firrtl/transforms/LegalizeClocksAndAsyncResets.scala +++ b/src/main/scala/firrtl/transforms/LegalizeClocksAndAsyncResets.scala @@ -91,7 +91,7 @@ class LegalizeClocksAndAsyncResetsTransform extends Transform with DependencyAPI Dependency[FixAddingNegativeLiterals], Dependency[ReplaceTruncatingArithmetic], Dependency[InlineBitExtractionsTransform], - Dependency[InlineCastsTransform] + Dependency[InlineAcrossCastsTransform] ) override def optionalPrerequisites = firrtl.stage.Forms.LowFormOptimized diff --git a/src/main/scala/firrtl/transforms/RemoveKeywordCollisions.scala b/src/main/scala/firrtl/transforms/RemoveKeywordCollisions.scala index 0bf6419f..69d4aa8d 100644 --- a/src/main/scala/firrtl/transforms/RemoveKeywordCollisions.scala +++ b/src/main/scala/firrtl/transforms/RemoveKeywordCollisions.scala @@ -37,7 +37,7 @@ class VerilogRename extends RemoveKeywordCollisions(v_keywords) { Dependency[FixAddingNegativeLiterals], Dependency[ReplaceTruncatingArithmetic], Dependency[InlineBitExtractionsTransform], - Dependency[InlineCastsTransform], + Dependency[InlineAcrossCastsTransform], Dependency[LegalizeClocksTransform], Dependency[FlattenRegUpdate], Dependency(passes.VerilogModulusCleanup) diff --git a/src/main/scala/firrtl/transforms/package.scala b/src/main/scala/firrtl/transforms/package.scala index d758fa0a..5455690e 100644 --- a/src/main/scala/firrtl/transforms/package.scala +++ b/src/main/scala/firrtl/transforms/package.scala @@ -3,6 +3,9 @@ package firrtl package object transforms { + type InlineAcrossCastsTransform = InlineCastsTransform + val InlineAcrossCastsTransform = InlineCastsTransform + @deprecated("Replaced by LegalizeClocksAndAsyncResetsTransform", "FIRRTL 1.4.0") type LegalizeClocksTransform = LegalizeClocksAndAsyncResetsTransform @deprecated("Replaced by LegalizeClocksAndAsyncResetsTransform", "FIRRTL 1.4.0") diff --git a/src/test/scala/firrtlTests/InlineAcrossCastsSpec.scala b/src/test/scala/firrtlTests/InlineAcrossCastsSpec.scala new file mode 100644 index 00000000..669ae077 --- /dev/null +++ b/src/test/scala/firrtlTests/InlineAcrossCastsSpec.scala @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: Apache-2.0 + +package firrtlTests + +import firrtl.transforms.InlineAcrossCastsTransform +import firrtl.testutils.FirrtlFlatSpec +import firrtl.testutils.FirrtlCheckers._ + +class InlineAcrossCastsEquivalenceSpec extends FirrtlFlatSpec { + /* + * Note: InlineCasts is still part of mverilog, so this test must both: + * - Test that the InlineCasts fix is effective given the current mverilog + * - Provide a test that will be robust if and when InlineCasts is no longer run in mverilog + * + * This is why the test passes InlineCasts as a custom transform: to future-proof it so that + * it can do real LEC against no-InlineCasts. It currently is just a sanity check that the + * emitted Verilog is legal, but it will automatically become a more meaningful test when + * InlineCasts is not run in mverilog. + */ + "InlineCastsTransform" should "not produce broken Verilog" in { + val input = + s"""circuit literalsel_fir: + | module literalsel_fir: + | input i: UInt<4> + | output o: SInt<8> + | o <= pad(asSInt(UInt<2>("h1")), 8) + |""".stripMargin + firrtlEquivalenceTest(input, Seq(new InlineAcrossCastsTransform)) + } + + it should "not inline complex expressions into other complex expressions" in { + val input = + """circuit NeverInlineComplexIntoComplex : + | module NeverInlineComplexIntoComplex : + | input a : SInt<3> + | input b : UInt<2> + | input c : UInt<2> + | input sel : UInt<1> + | output out : SInt<3> + | node diff = sub(b, c) + | out <= mux(sel, a, asSInt(diff)) + |""".stripMargin + val expected = + """module NeverInlineComplexIntoComplexRef( + | input [2:0] a, + | input [1:0] b, + | input [1:0] c, + | input sel, + | output [2:0] out + |); + | wire [2:0] diff = b - c; + | assign out = sel ? $signed(a) : $signed(diff); + |endmodule + |""".stripMargin + firrtlEquivalenceWithVerilog(input, expected) + } + + it should "inline casts on both sides of a more complex expression" in { + val input = + """circuit test : + | module test : + | input clock : Clock + | input in : UInt<8> + | output out : UInt<8> + | + | node _T_1 = asUInt(clock) + | node _T_2 = not(_T_1) + | node clock_n = asClock(_T_2) + | reg r : UInt<8>, clock_n + | r <= in + | out <= r + |""".stripMargin + val verilog = compileToVerilogCircuitState(input) + verilog should containLine("always @(posedge clock_n) begin") + + } +} diff --git a/src/test/scala/firrtlTests/InlineCastsSpec.scala b/src/test/scala/firrtlTests/InlineCastsSpec.scala deleted file mode 100644 index 7a248def..00000000 --- a/src/test/scala/firrtlTests/InlineCastsSpec.scala +++ /dev/null @@ -1,77 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 - -package firrtlTests - -import firrtl.transforms.InlineCastsTransform -import firrtl.testutils.FirrtlFlatSpec -import firrtl.testutils.FirrtlCheckers._ - -class InlineCastsEquivalenceSpec extends FirrtlFlatSpec { - /* - * Note: InlineCasts is still part of mverilog, so this test must both: - * - Test that the InlineCasts fix is effective given the current mverilog - * - Provide a test that will be robust if and when InlineCasts is no longer run in mverilog - * - * This is why the test passes InlineCasts as a custom transform: to future-proof it so that - * it can do real LEC against no-InlineCasts. It currently is just a sanity check that the - * emitted Verilog is legal, but it will automatically become a more meaningful test when - * InlineCasts is not run in mverilog. - */ - "InlineCastsTransform" should "not produce broken Verilog" in { - val input = - s"""circuit literalsel_fir: - | module literalsel_fir: - | input i: UInt<4> - | output o: SInt<8> - | o <= pad(asSInt(UInt<2>("h1")), 8) - |""".stripMargin - firrtlEquivalenceTest(input, Seq(new InlineCastsTransform)) - } - - it should "not inline complex expressions into other complex expressions" in { - val input = - """circuit NeverInlineComplexIntoComplex : - | module NeverInlineComplexIntoComplex : - | input a : SInt<3> - | input b : UInt<2> - | input c : UInt<2> - | input sel : UInt<1> - | output out : SInt<3> - | node diff = sub(b, c) - | out <= mux(sel, a, asSInt(diff)) - |""".stripMargin - val expected = - """module NeverInlineComplexIntoComplexRef( - | input [2:0] a, - | input [1:0] b, - | input [1:0] c, - | input sel, - | output [2:0] out - |); - | wire [2:0] diff = b - c; - | assign out = sel ? $signed(a) : $signed(diff); - |endmodule - |""".stripMargin - firrtlEquivalenceWithVerilog(input, expected) - } - - it should "inline casts on both sides of a more complex expression" in { - val input = - """circuit test : - | module test : - | input clock : Clock - | input in : UInt<8> - | output out : UInt<8> - | - | node _T_1 = asUInt(clock) - | node _T_2 = not(_T_1) - | node clock_n = asClock(_T_2) - | reg r : UInt<8>, clock_n - | r <= in - | out <= r - |""".stripMargin - val verilog = compileToVerilogCircuitState(input) - verilog should containLine("always @(posedge clock_n) begin") - - } -} diff --git a/src/test/scala/firrtlTests/LoweringCompilersSpec.scala b/src/test/scala/firrtlTests/LoweringCompilersSpec.scala index bdc72e7b..d56ca657 100644 --- a/src/test/scala/firrtlTests/LoweringCompilersSpec.scala +++ b/src/test/scala/firrtlTests/LoweringCompilersSpec.scala @@ -247,7 +247,7 @@ class LoweringCompilersSpec extends AnyFlatSpec with Matchers { new firrtl.transforms.ReplaceTruncatingArithmetic, new firrtl.transforms.InlineBitExtractionsTransform, new firrtl.transforms.PropagatePresetAnnotations, - new firrtl.transforms.InlineCastsTransform, + new firrtl.transforms.InlineAcrossCastsTransform, new firrtl.transforms.LegalizeClocksTransform, new firrtl.transforms.FlattenRegUpdate, firrtl.passes.VerilogModulusCleanup, @@ -271,7 +271,7 @@ class LoweringCompilersSpec extends AnyFlatSpec with Matchers { new firrtl.transforms.ReplaceTruncatingArithmetic, new firrtl.transforms.InlineBitExtractionsTransform, new firrtl.transforms.PropagatePresetAnnotations, - new firrtl.transforms.InlineCastsTransform, + new firrtl.transforms.InlineAcrossCastsTransform, new firrtl.transforms.LegalizeClocksTransform, new firrtl.transforms.FlattenRegUpdate, new firrtl.transforms.DeadCodeElimination, -- cgit v1.2.3