From a074e555b3bf1106bc91bed06cf097bc48a4690d Mon Sep 17 00:00:00 2001 From: Aditya Naik Date: Thu, 27 Jun 2024 12:24:35 -0700 Subject: [PATCH 1/5] Straigthen reversals for elements handling --- core/src/main/scala/chisel3/Aggregate.scala | 2 - .../chisel3/internal/firrtl/Converter.scala | 2 +- .../internal/plugin/BundleComponent.scala | 4 +- .../chiselTests/BundleElementsSpec.scala | 141 ++++++++++++------ 4 files changed, 96 insertions(+), 53 deletions(-) diff --git a/core/src/main/scala/chisel3/Aggregate.scala b/core/src/main/scala/chisel3/Aggregate.scala index e0fccbbee6b..41277426d70 100644 --- a/core/src/main/scala/chisel3/Aggregate.scala +++ b/core/src/main/scala/chisel3/Aggregate.scala @@ -1493,8 +1493,6 @@ abstract class Bundle extends Record { case (name, Some(data: Data)) => Some(name -> data) case _ => None - }.sortWith { - case ((an, a), (bn, b)) => (a._id > b._id) || ((a eq b) && (an > bn)) }: _*) } diff --git a/core/src/main/scala/chisel3/internal/firrtl/Converter.scala b/core/src/main/scala/chisel3/internal/firrtl/Converter.scala index 74fa57dbe46..f701676b964 100644 --- a/core/src/main/scala/chisel3/internal/firrtl/Converter.scala +++ b/core/src/main/scala/chisel3/internal/firrtl/Converter.scala @@ -410,7 +410,7 @@ private[chisel3] object Converter { fir.Field(getRef(elt, info).name, fir.Flip, extractType(elt, false, info, checkProbe, true, typeAliases)) } if (!t._isOpaqueType) - fir.BundleType(t._elements.toIndexedSeq.reverse.map { case (_, e) => eltField(e) }) + fir.BundleType(t._elements.toIndexedSeq.map { case (_, e) => eltField(e) }) else extractType(t._elements.head._2, childClearDir, info, checkProbe, true, typeAliases) } diff --git a/plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala b/plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala index 382a9d1d0e2..4608597fc8e 100644 --- a/plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala +++ b/plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala @@ -139,7 +139,6 @@ private[plugin] class BundleComponent(val global: Global, arguments: ChiselPlugi } val currentFields = bundleSymbol.info.members.flatMap { - case member if member.isPublic => if (isBundleField(member)) { // The params have spaces after them (Scalac implementation detail) @@ -147,9 +146,8 @@ private[plugin] class BundleComponent(val global: Global, arguments: ChiselPlugi } else { None } - case _ => None - }.toList + }.toList.reverse val allParentFields = bundleSymbol.parentSymbols.flatMap { parentSymbol => val fieldsFromParent = if (depth < 1 && !isExactBundle(bundleSymbol)) { diff --git a/src/test/scala/chiselTests/BundleElementsSpec.scala b/src/test/scala/chiselTests/BundleElementsSpec.scala index 2b066f102fb..02673c31d6e 100644 --- a/src/test/scala/chiselTests/BundleElementsSpec.scala +++ b/src/test/scala/chiselTests/BundleElementsSpec.scala @@ -44,13 +44,13 @@ class BpipDecoupled extends BpipOneField { class HasDecoupledBundleByInheritance extends Module { val out1 = IO(Output(new BpipDecoupled)) assertElementsMatchExpected(out1)( - "bpipDecoupledDecoupled" -> _.bpipDecoupledDecoupled, - "bpipDecoupledVec" -> _.bpipDecoupledVec, - "bpipDecoupledSInt" -> _.bpipDecoupledSInt, - "bpipOneFieldTwo" -> _.bpipOneFieldTwo, - "bpipOneFieldOne" -> _.bpipOneFieldOne, + "bpipSuperTraitGood" -> _.bpipSuperTraitGood, "bpipTraitGood" -> _.bpipTraitGood, - "bpipSuperTraitGood" -> _.bpipSuperTraitGood + "bpipOneFieldOne" -> _.bpipOneFieldOne, + "bpipOneFieldTwo" -> _.bpipOneFieldTwo, + "bpipDecoupledSInt" -> _.bpipDecoupledSInt, + "bpipDecoupledVec" -> _.bpipDecoupledVec, + "bpipDecoupledDecoupled" -> _.bpipDecoupledDecoupled, ) } @@ -58,13 +58,13 @@ class HasDecoupledBundleByInheritance extends Module { class DebugProblem3 extends Module { val out1 = IO(Output(new BpipTwoField)) assertElementsMatchExpected(out1)( - "baz" -> _.baz, - "bpipTwoFieldTwo" -> _.bpipTwoFieldTwo, - "bpipTwoFieldOne" -> _.bpipTwoFieldOne, - "bpipOneFieldTwo" -> _.bpipOneFieldTwo, - "bpipOneFieldOne" -> _.bpipOneFieldOne, + "bpipSuperTraitGood" -> _.bpipSuperTraitGood, "bpipTraitGood" -> _.bpipTraitGood, - "bpipSuperTraitGood" -> _.bpipSuperTraitGood + "bpipOneFieldOne" -> _.bpipOneFieldOne, + "bpipOneFieldTwo" -> _.bpipOneFieldTwo, + "bpipTwoFieldOne" -> _.bpipTwoFieldOne, + "bpipTwoFieldTwo" -> _.bpipTwoFieldTwo, + "baz" -> _.baz, ) } @@ -107,12 +107,12 @@ class ForFieldOrderingTest extends Module { val out1 = IO(Output(new BpipP8_3)) out1 := DontCare assertElementsMatchExpected(out1)( - "field_3_2" -> _.field_3_2, - "field_3_1" -> _.field_3_1, - "field_2_2" -> _.field_2_2, - "field_2_1" -> _.field_2_1, + "field_1_1" -> _.field_1_1, "field_1_2" -> _.field_1_2, - "field_1_1" -> _.field_1_1 + "field_2_1" -> _.field_2_1, + "field_2_2" -> _.field_2_2, + "field_3_1" -> _.field_3_1, + "field_3_2" -> _.field_3_2, ) } @@ -152,11 +152,11 @@ class HasGenParamsPassedToSuperclasses extends Module { out1 := DontCare assertElementsMatchExpected(out1)( - "baz" -> _.baz, - "qux" -> _.qux, - "bar" -> _.bar, + "superFoo" -> _.superFoo, "superQux" -> _.superQux, - "superFoo" -> _.superFoo + "bar" -> _.bar, + "qux" -> _.qux, + "baz" -> _.baz, ) } @@ -207,7 +207,44 @@ class UsesBundleWithGeneratorField extends Module { out := DontCare - assertElementsMatchExpected(out)("superQux" -> _.superQux, "superFoo" -> _.superFoo) + assertElementsMatchExpected(out)( + "superFoo" -> _.superFoo, + "superQux" -> _.superQux + ) +} + + +case class GenericBundle[T <: Data, U <: Data](val a: T, val b: U) extends Bundle + +class SimpleBundleElemOrder extends Module { + val in1 = IO(Input(GenericBundle(a = Bool(), b = UInt(8.W)))) + val in2 = IO(Input(GenericBundle(b = UInt(8.W), a = Bool()))) + val out1 = IO(Output(UInt(in1.getWidth.W))) + val out2 = IO(Output(UInt(in2.getWidth.W))) + out1 := in1.asUInt + out2 := in2.asUInt +} + +class BundleElemOrder1(gen: => UInt) extends Bundle { + val a = UInt(8.W) + val b = gen + val c = UInt(8.W) + +} +class BundleElemOrder2(gen: UInt) extends Bundle { + val a = UInt(8.W) + val b = gen + val c = UInt(8.W) + +} + +class SimpleBundleElemOrderByName extends Module { + val in0 = IO(Input(new BundleElemOrder1(UInt(8.W)))) + val in1 = IO(Input(new BundleElemOrder2(UInt(8.W)))) + val out0 = IO(Output(UInt(24.W))) + val out1 = IO(Output(UInt(24.W))) + out0 := in0.asUInt + out1 := in1.asUInt } /* Testing whether gen fields superFoo and superQux can be found when they are @@ -271,18 +308,18 @@ class BundleElementsSpec extends AnyFreeSpec with Matchers { out5 := DontCare assertElementsMatchExpected(out)( - "animals" -> _.animals, - "baz" -> _.baz, - "bar" -> _.bar, - "varmint" -> _.varmint, - "fieldThree" -> _.fieldThree, - "fieldTwo" -> _.fieldTwo, "fieldOne" -> _.fieldOne, - "foo" -> _.foo + "fieldTwo" -> _.fieldTwo, + "fieldThree" -> _.fieldThree, + "varmint" -> _.varmint, + "foo" -> _.foo, + "bar" -> _.bar, + "baz" -> _.baz, + "animals" -> _.animals, ) - assertElementsMatchExpected(out5)("fieldThree" -> _.fieldThree, "fieldTwo" -> _.fieldTwo, "fieldOne" -> _.fieldOne) - assertElementsMatchExpected(out2)("notAbstract" -> _.notAbstract, "fromAbstractBundle" -> _.fromAbstractBundle) - assertElementsMatchExpected(out4)("fox" -> _.fox, "dog" -> _.dog) + assertElementsMatchExpected(out5)("fieldOne" -> _.fieldOne, "fieldTwo" -> _.fieldTwo, "fieldThree" -> _.fieldThree) + assertElementsMatchExpected(out2)("fromAbstractBundle" -> _.fromAbstractBundle, "notAbstract" -> _.notAbstract) + assertElementsMatchExpected(out4)("dog" -> _.dog, "fox" -> _.fox) } "Complex Bundle with inheritance, traits and params. DebugProblem1" in { @@ -351,7 +388,7 @@ class BundleElementsSpec extends AnyFreeSpec with Matchers { val outTrue = IO(Output(new OptionBundle(hasIn = true))) val outFalse = IO(Output(new OptionBundle(hasIn = false))) //NOTE: The _.in.get _.in is an optional field - assertElementsMatchExpected(outTrue)("out" -> _.out, "in" -> _.in.get) + assertElementsMatchExpected(outTrue)("in" -> _.in.get, "out" -> _.out) assertElementsMatchExpected(outFalse)("out" -> _.out) } @@ -370,7 +407,7 @@ class BundleElementsSpec extends AnyFreeSpec with Matchers { val b = Bool() val c = Enum0.Type })) - assertElementsMatchExpected(out)("b" -> _.b, "a" -> _.a) + assertElementsMatchExpected(out)( "a" -> _.a, "b" -> _.b) }) } @@ -406,25 +443,25 @@ class BundleElementsSpec extends AnyFreeSpec with Matchers { } "one of the expected data values is wrong" in { - checkAssertion("b" -> _.b, "a" -> _.b)("field 'a' data field BundleElementsSpec_Anon.out.a") + checkAssertion("a" -> _.b, "b" -> _.b)("field 'a' data field BundleElementsSpec_Anon.out.a") } "one of the expected field names in wrong" in { - checkAssertion("b" -> _.b, "z" -> _.a)("field: 'a' did not match expected 'z'") + checkAssertion("z" -> _.a, "b" -> _.b)("field: 'a' did not match expected 'z'") } "fields that are expected are not returned by the elements method" in { - checkAssertion("b" -> _.b, "a" -> _.a, "c" -> _.a)("#elements is missing the 'c' field") + checkAssertion("a" -> _.a, "b" -> _.b, "c" -> _.a)("#elements is missing the 'c' field") } "fields returned by the element are not specified in the expected fields" in { - checkAssertion("b" -> _.b)("expected fields did not include 'a' field found in #elements") + checkAssertion("a" -> _.a)("expected fields did not include 'b' field found in #elements") } "multiple errors between elements method and expected fields are shown in the assertion error message" in { checkAssertion()( - "expected fields did not include 'b' field found in #elements," + - " expected fields did not include 'a' field found in #elements" + "expected fields did not include 'a' field found in #elements," + + " expected fields did not include 'b' field found in #elements" ) } } @@ -437,7 +474,7 @@ class BundleElementsSpec extends AnyFreeSpec with Matchers { val y = if (false) Some(Input(UInt(8.W))) else None } }) - assertElementsMatchExpected(io)("x" -> _.x, "foo" -> _.foo) + assertElementsMatchExpected(io)("foo" -> _.foo, "x" -> _.x) assertElementsMatchExpected(io.x)() }) } @@ -469,16 +506,26 @@ class BundleElementsSpec extends AnyFreeSpec with Matchers { val io = IO(new BpipOptionBundle) assertElementsMatchExpected(io)( + "bpipUIntVal" -> _.bpipUIntVal, "bpipUIntLazyVal" -> _.bpipUIntLazyVal, - "bpipOptionUInt" -> _.bpipOptionUInt.get, "bpipUIntVar" -> _.bpipUIntVar, - "bpipUIntVal" -> _.bpipUIntVal + "bpipOptionUInt" -> _.bpipOptionUInt.get, ) } ChiselStage.emitCHIRRTL(new ALU(ALUConfig(10, mul = true, b = false))) } + "Bundle elements should be ordered in order of declaration" in { + val chirrtl = ChiselStage.emitCHIRRTL(new SimpleBundleElemOrder) + chirrtl should include("input in1 : { a : UInt<1>, b : UInt<8>}") + chirrtl should include("input in2 : { a : UInt<1>, b : UInt<8>}") + + val chirrtl2 = ChiselStage.emitCHIRRTL(new SimpleBundleElemOrderByName) + chirrtl2 should include("input in0 : { a : UInt<8>, b : UInt<8>, c : UInt<8>}") + chirrtl2 should include("input in1 : { a : UInt<8>, b : UInt<8>, c : UInt<8>}") + } + "TraceSpec test, different version found in TraceSpec.scala" in { class Bundle0 extends Bundle { val a = UInt(8.W) @@ -498,9 +545,9 @@ class BundleElementsSpec extends AnyFreeSpec with Matchers { o := r r := i - assertElementsMatchExpected(i)("b" -> _.b, "a" -> _.a) - assertElementsMatchExpected(o)("b" -> _.b, "a" -> _.a) - assertElementsMatchExpected(r)("b" -> _.b, "a" -> _.a) + assertElementsMatchExpected(i)("a" -> _.a, "b" -> _.b) + assertElementsMatchExpected(o)("a" -> _.a, "b" -> _.b) + assertElementsMatchExpected(r)("a" -> _.a, "b" -> _.b) } class Module1 extends Module { @@ -508,7 +555,7 @@ class BundleElementsSpec extends AnyFreeSpec with Matchers { val m0 = Module(new Module0) m0.i := i m0.o := DontCare - assertElementsMatchExpected(i)("b" -> _.b, "a" -> _.a) + assertElementsMatchExpected(i)( "a" -> _.a, "b" -> _.b) } object Enum0 extends ChiselEnum { From f46eabecc5afb5bc914425da9c8eb9052a646442 Mon Sep 17 00:00:00 2001 From: Aditya Naik Date: Thu, 27 Jun 2024 12:29:04 -0700 Subject: [PATCH 2/5] Remove reversal for printable --- core/src/main/scala/chisel3/Aggregate.scala | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/core/src/main/scala/chisel3/Aggregate.scala b/core/src/main/scala/chisel3/Aggregate.scala index 41277426d70..e76918ce026 100644 --- a/core/src/main/scala/chisel3/Aggregate.scala +++ b/core/src/main/scala/chisel3/Aggregate.scala @@ -1519,9 +1519,7 @@ abstract class Bundle extends Record { /** Default "pretty-print" implementation * Analogous to printing a Map * Results in "`Bundle(elt0.name -> elt0.value, ...)`" - * @note The order is reversed from the order of elements in order to print - * the fields in the order they were defined */ - override def toPrintable: Printable = toPrintableHelper(_elements.toList.reverse) + override def toPrintable: Printable = toPrintableHelper(_elements.toList) } From 3c230c1373ca9f221b548adf73b22c0bc93f5eea Mon Sep 17 00:00:00 2001 From: Aditya Naik Date: Wed, 24 Jul 2024 13:17:05 -0700 Subject: [PATCH 3/5] Undo reversal changes --- core/src/main/scala/chisel3/Aggregate.scala | 4 +- .../chisel3/internal/firrtl/Converter.scala | 2 +- .../internal/plugin/BundleComponent.scala | 4 +- .../chiselTests/BundleElementsSpec.scala | 141 ++++++------------ 4 files changed, 54 insertions(+), 97 deletions(-) diff --git a/core/src/main/scala/chisel3/Aggregate.scala b/core/src/main/scala/chisel3/Aggregate.scala index e76918ce026..41277426d70 100644 --- a/core/src/main/scala/chisel3/Aggregate.scala +++ b/core/src/main/scala/chisel3/Aggregate.scala @@ -1519,7 +1519,9 @@ abstract class Bundle extends Record { /** Default "pretty-print" implementation * Analogous to printing a Map * Results in "`Bundle(elt0.name -> elt0.value, ...)`" + * @note The order is reversed from the order of elements in order to print + * the fields in the order they were defined */ - override def toPrintable: Printable = toPrintableHelper(_elements.toList) + override def toPrintable: Printable = toPrintableHelper(_elements.toList.reverse) } diff --git a/core/src/main/scala/chisel3/internal/firrtl/Converter.scala b/core/src/main/scala/chisel3/internal/firrtl/Converter.scala index f701676b964..74fa57dbe46 100644 --- a/core/src/main/scala/chisel3/internal/firrtl/Converter.scala +++ b/core/src/main/scala/chisel3/internal/firrtl/Converter.scala @@ -410,7 +410,7 @@ private[chisel3] object Converter { fir.Field(getRef(elt, info).name, fir.Flip, extractType(elt, false, info, checkProbe, true, typeAliases)) } if (!t._isOpaqueType) - fir.BundleType(t._elements.toIndexedSeq.map { case (_, e) => eltField(e) }) + fir.BundleType(t._elements.toIndexedSeq.reverse.map { case (_, e) => eltField(e) }) else extractType(t._elements.head._2, childClearDir, info, checkProbe, true, typeAliases) } diff --git a/plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala b/plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala index 4608597fc8e..382a9d1d0e2 100644 --- a/plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala +++ b/plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala @@ -139,6 +139,7 @@ private[plugin] class BundleComponent(val global: Global, arguments: ChiselPlugi } val currentFields = bundleSymbol.info.members.flatMap { + case member if member.isPublic => if (isBundleField(member)) { // The params have spaces after them (Scalac implementation detail) @@ -146,8 +147,9 @@ private[plugin] class BundleComponent(val global: Global, arguments: ChiselPlugi } else { None } + case _ => None - }.toList.reverse + }.toList val allParentFields = bundleSymbol.parentSymbols.flatMap { parentSymbol => val fieldsFromParent = if (depth < 1 && !isExactBundle(bundleSymbol)) { diff --git a/src/test/scala/chiselTests/BundleElementsSpec.scala b/src/test/scala/chiselTests/BundleElementsSpec.scala index 02673c31d6e..2b066f102fb 100644 --- a/src/test/scala/chiselTests/BundleElementsSpec.scala +++ b/src/test/scala/chiselTests/BundleElementsSpec.scala @@ -44,13 +44,13 @@ class BpipDecoupled extends BpipOneField { class HasDecoupledBundleByInheritance extends Module { val out1 = IO(Output(new BpipDecoupled)) assertElementsMatchExpected(out1)( - "bpipSuperTraitGood" -> _.bpipSuperTraitGood, - "bpipTraitGood" -> _.bpipTraitGood, - "bpipOneFieldOne" -> _.bpipOneFieldOne, - "bpipOneFieldTwo" -> _.bpipOneFieldTwo, - "bpipDecoupledSInt" -> _.bpipDecoupledSInt, - "bpipDecoupledVec" -> _.bpipDecoupledVec, "bpipDecoupledDecoupled" -> _.bpipDecoupledDecoupled, + "bpipDecoupledVec" -> _.bpipDecoupledVec, + "bpipDecoupledSInt" -> _.bpipDecoupledSInt, + "bpipOneFieldTwo" -> _.bpipOneFieldTwo, + "bpipOneFieldOne" -> _.bpipOneFieldOne, + "bpipTraitGood" -> _.bpipTraitGood, + "bpipSuperTraitGood" -> _.bpipSuperTraitGood ) } @@ -58,13 +58,13 @@ class HasDecoupledBundleByInheritance extends Module { class DebugProblem3 extends Module { val out1 = IO(Output(new BpipTwoField)) assertElementsMatchExpected(out1)( - "bpipSuperTraitGood" -> _.bpipSuperTraitGood, - "bpipTraitGood" -> _.bpipTraitGood, - "bpipOneFieldOne" -> _.bpipOneFieldOne, - "bpipOneFieldTwo" -> _.bpipOneFieldTwo, - "bpipTwoFieldOne" -> _.bpipTwoFieldOne, - "bpipTwoFieldTwo" -> _.bpipTwoFieldTwo, "baz" -> _.baz, + "bpipTwoFieldTwo" -> _.bpipTwoFieldTwo, + "bpipTwoFieldOne" -> _.bpipTwoFieldOne, + "bpipOneFieldTwo" -> _.bpipOneFieldTwo, + "bpipOneFieldOne" -> _.bpipOneFieldOne, + "bpipTraitGood" -> _.bpipTraitGood, + "bpipSuperTraitGood" -> _.bpipSuperTraitGood ) } @@ -107,12 +107,12 @@ class ForFieldOrderingTest extends Module { val out1 = IO(Output(new BpipP8_3)) out1 := DontCare assertElementsMatchExpected(out1)( - "field_1_1" -> _.field_1_1, - "field_1_2" -> _.field_1_2, - "field_2_1" -> _.field_2_1, - "field_2_2" -> _.field_2_2, - "field_3_1" -> _.field_3_1, "field_3_2" -> _.field_3_2, + "field_3_1" -> _.field_3_1, + "field_2_2" -> _.field_2_2, + "field_2_1" -> _.field_2_1, + "field_1_2" -> _.field_1_2, + "field_1_1" -> _.field_1_1 ) } @@ -152,11 +152,11 @@ class HasGenParamsPassedToSuperclasses extends Module { out1 := DontCare assertElementsMatchExpected(out1)( - "superFoo" -> _.superFoo, - "superQux" -> _.superQux, - "bar" -> _.bar, - "qux" -> _.qux, "baz" -> _.baz, + "qux" -> _.qux, + "bar" -> _.bar, + "superQux" -> _.superQux, + "superFoo" -> _.superFoo ) } @@ -207,44 +207,7 @@ class UsesBundleWithGeneratorField extends Module { out := DontCare - assertElementsMatchExpected(out)( - "superFoo" -> _.superFoo, - "superQux" -> _.superQux - ) -} - - -case class GenericBundle[T <: Data, U <: Data](val a: T, val b: U) extends Bundle - -class SimpleBundleElemOrder extends Module { - val in1 = IO(Input(GenericBundle(a = Bool(), b = UInt(8.W)))) - val in2 = IO(Input(GenericBundle(b = UInt(8.W), a = Bool()))) - val out1 = IO(Output(UInt(in1.getWidth.W))) - val out2 = IO(Output(UInt(in2.getWidth.W))) - out1 := in1.asUInt - out2 := in2.asUInt -} - -class BundleElemOrder1(gen: => UInt) extends Bundle { - val a = UInt(8.W) - val b = gen - val c = UInt(8.W) - -} -class BundleElemOrder2(gen: UInt) extends Bundle { - val a = UInt(8.W) - val b = gen - val c = UInt(8.W) - -} - -class SimpleBundleElemOrderByName extends Module { - val in0 = IO(Input(new BundleElemOrder1(UInt(8.W)))) - val in1 = IO(Input(new BundleElemOrder2(UInt(8.W)))) - val out0 = IO(Output(UInt(24.W))) - val out1 = IO(Output(UInt(24.W))) - out0 := in0.asUInt - out1 := in1.asUInt + assertElementsMatchExpected(out)("superQux" -> _.superQux, "superFoo" -> _.superFoo) } /* Testing whether gen fields superFoo and superQux can be found when they are @@ -308,18 +271,18 @@ class BundleElementsSpec extends AnyFreeSpec with Matchers { out5 := DontCare assertElementsMatchExpected(out)( - "fieldOne" -> _.fieldOne, - "fieldTwo" -> _.fieldTwo, - "fieldThree" -> _.fieldThree, - "varmint" -> _.varmint, - "foo" -> _.foo, - "bar" -> _.bar, - "baz" -> _.baz, "animals" -> _.animals, + "baz" -> _.baz, + "bar" -> _.bar, + "varmint" -> _.varmint, + "fieldThree" -> _.fieldThree, + "fieldTwo" -> _.fieldTwo, + "fieldOne" -> _.fieldOne, + "foo" -> _.foo ) - assertElementsMatchExpected(out5)("fieldOne" -> _.fieldOne, "fieldTwo" -> _.fieldTwo, "fieldThree" -> _.fieldThree) - assertElementsMatchExpected(out2)("fromAbstractBundle" -> _.fromAbstractBundle, "notAbstract" -> _.notAbstract) - assertElementsMatchExpected(out4)("dog" -> _.dog, "fox" -> _.fox) + assertElementsMatchExpected(out5)("fieldThree" -> _.fieldThree, "fieldTwo" -> _.fieldTwo, "fieldOne" -> _.fieldOne) + assertElementsMatchExpected(out2)("notAbstract" -> _.notAbstract, "fromAbstractBundle" -> _.fromAbstractBundle) + assertElementsMatchExpected(out4)("fox" -> _.fox, "dog" -> _.dog) } "Complex Bundle with inheritance, traits and params. DebugProblem1" in { @@ -388,7 +351,7 @@ class BundleElementsSpec extends AnyFreeSpec with Matchers { val outTrue = IO(Output(new OptionBundle(hasIn = true))) val outFalse = IO(Output(new OptionBundle(hasIn = false))) //NOTE: The _.in.get _.in is an optional field - assertElementsMatchExpected(outTrue)("in" -> _.in.get, "out" -> _.out) + assertElementsMatchExpected(outTrue)("out" -> _.out, "in" -> _.in.get) assertElementsMatchExpected(outFalse)("out" -> _.out) } @@ -407,7 +370,7 @@ class BundleElementsSpec extends AnyFreeSpec with Matchers { val b = Bool() val c = Enum0.Type })) - assertElementsMatchExpected(out)( "a" -> _.a, "b" -> _.b) + assertElementsMatchExpected(out)("b" -> _.b, "a" -> _.a) }) } @@ -443,25 +406,25 @@ class BundleElementsSpec extends AnyFreeSpec with Matchers { } "one of the expected data values is wrong" in { - checkAssertion("a" -> _.b, "b" -> _.b)("field 'a' data field BundleElementsSpec_Anon.out.a") + checkAssertion("b" -> _.b, "a" -> _.b)("field 'a' data field BundleElementsSpec_Anon.out.a") } "one of the expected field names in wrong" in { - checkAssertion("z" -> _.a, "b" -> _.b)("field: 'a' did not match expected 'z'") + checkAssertion("b" -> _.b, "z" -> _.a)("field: 'a' did not match expected 'z'") } "fields that are expected are not returned by the elements method" in { - checkAssertion("a" -> _.a, "b" -> _.b, "c" -> _.a)("#elements is missing the 'c' field") + checkAssertion("b" -> _.b, "a" -> _.a, "c" -> _.a)("#elements is missing the 'c' field") } "fields returned by the element are not specified in the expected fields" in { - checkAssertion("a" -> _.a)("expected fields did not include 'b' field found in #elements") + checkAssertion("b" -> _.b)("expected fields did not include 'a' field found in #elements") } "multiple errors between elements method and expected fields are shown in the assertion error message" in { checkAssertion()( - "expected fields did not include 'a' field found in #elements," + - " expected fields did not include 'b' field found in #elements" + "expected fields did not include 'b' field found in #elements," + + " expected fields did not include 'a' field found in #elements" ) } } @@ -474,7 +437,7 @@ class BundleElementsSpec extends AnyFreeSpec with Matchers { val y = if (false) Some(Input(UInt(8.W))) else None } }) - assertElementsMatchExpected(io)("foo" -> _.foo, "x" -> _.x) + assertElementsMatchExpected(io)("x" -> _.x, "foo" -> _.foo) assertElementsMatchExpected(io.x)() }) } @@ -506,26 +469,16 @@ class BundleElementsSpec extends AnyFreeSpec with Matchers { val io = IO(new BpipOptionBundle) assertElementsMatchExpected(io)( - "bpipUIntVal" -> _.bpipUIntVal, "bpipUIntLazyVal" -> _.bpipUIntLazyVal, - "bpipUIntVar" -> _.bpipUIntVar, "bpipOptionUInt" -> _.bpipOptionUInt.get, + "bpipUIntVar" -> _.bpipUIntVar, + "bpipUIntVal" -> _.bpipUIntVal ) } ChiselStage.emitCHIRRTL(new ALU(ALUConfig(10, mul = true, b = false))) } - "Bundle elements should be ordered in order of declaration" in { - val chirrtl = ChiselStage.emitCHIRRTL(new SimpleBundleElemOrder) - chirrtl should include("input in1 : { a : UInt<1>, b : UInt<8>}") - chirrtl should include("input in2 : { a : UInt<1>, b : UInt<8>}") - - val chirrtl2 = ChiselStage.emitCHIRRTL(new SimpleBundleElemOrderByName) - chirrtl2 should include("input in0 : { a : UInt<8>, b : UInt<8>, c : UInt<8>}") - chirrtl2 should include("input in1 : { a : UInt<8>, b : UInt<8>, c : UInt<8>}") - } - "TraceSpec test, different version found in TraceSpec.scala" in { class Bundle0 extends Bundle { val a = UInt(8.W) @@ -545,9 +498,9 @@ class BundleElementsSpec extends AnyFreeSpec with Matchers { o := r r := i - assertElementsMatchExpected(i)("a" -> _.a, "b" -> _.b) - assertElementsMatchExpected(o)("a" -> _.a, "b" -> _.b) - assertElementsMatchExpected(r)("a" -> _.a, "b" -> _.b) + assertElementsMatchExpected(i)("b" -> _.b, "a" -> _.a) + assertElementsMatchExpected(o)("b" -> _.b, "a" -> _.a) + assertElementsMatchExpected(r)("b" -> _.b, "a" -> _.a) } class Module1 extends Module { @@ -555,7 +508,7 @@ class BundleElementsSpec extends AnyFreeSpec with Matchers { val m0 = Module(new Module0) m0.i := i m0.o := DontCare - assertElementsMatchExpected(i)( "a" -> _.a, "b" -> _.b) + assertElementsMatchExpected(i)("b" -> _.b, "a" -> _.a) } object Enum0 extends ChiselEnum { From 3edcb2a72139e6b77e030ecdc2501569a788e2ca Mon Sep 17 00:00:00 2001 From: Aditya Naik Date: Wed, 31 Jul 2024 11:28:08 -0700 Subject: [PATCH 4/5] Fix tests Add reverse to correct order --- core/src/main/scala/chisel3/Aggregate.scala | 2 +- .../internal/plugin/BundleComponent.scala | 2 +- .../chiselTests/BundleElementsSpec.scala | 54 +++++++++++++++++-- 3 files changed, 51 insertions(+), 7 deletions(-) diff --git a/core/src/main/scala/chisel3/Aggregate.scala b/core/src/main/scala/chisel3/Aggregate.scala index 41277426d70..ca6931bb3b6 100644 --- a/core/src/main/scala/chisel3/Aggregate.scala +++ b/core/src/main/scala/chisel3/Aggregate.scala @@ -1493,7 +1493,7 @@ abstract class Bundle extends Record { case (name, Some(data: Data)) => Some(name -> data) case _ => None - }: _*) + }.reverse: _*) } /** This method is implemented by the compiler plugin diff --git a/plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala b/plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala index 382a9d1d0e2..73ec1a82e51 100644 --- a/plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala +++ b/plugin/src/main/scala/chisel3/internal/plugin/BundleComponent.scala @@ -149,7 +149,7 @@ private[plugin] class BundleComponent(val global: Global, arguments: ChiselPlugi } case _ => None - }.toList + }.toList.reverse val allParentFields = bundleSymbol.parentSymbols.flatMap { parentSymbol => val fieldsFromParent = if (depth < 1 && !isExactBundle(bundleSymbol)) { diff --git a/src/test/scala/chiselTests/BundleElementsSpec.scala b/src/test/scala/chiselTests/BundleElementsSpec.scala index 2b066f102fb..41556b41b64 100644 --- a/src/test/scala/chiselTests/BundleElementsSpec.scala +++ b/src/test/scala/chiselTests/BundleElementsSpec.scala @@ -125,8 +125,9 @@ class HasValParamsToBundle extends Module { val out3 = IO(Output(new BpipParamIsField1(UInt(10.W)))) val out4 = IO(Output(new BpipParamIsField1(UInt(10.W)))) out3 := DontCare - assertElementsMatchExpected(out3)("paramField0" -> _.paramField0, "paramField1" -> _.paramField1) - assertElementsMatchExpected(out4)("paramField0" -> _.paramField0, "paramField1" -> _.paramField1) + + assertElementsMatchExpected(out3)("paramField1" -> _.paramField1, "paramField0" -> _.paramField0) + assertElementsMatchExpected(out4)("paramField1" -> _.paramField1, "paramField0" -> _.paramField0) } class HasGenParamsPassedToSuperclasses extends Module { @@ -210,6 +211,39 @@ class UsesBundleWithGeneratorField extends Module { assertElementsMatchExpected(out)("superQux" -> _.superQux, "superFoo" -> _.superFoo) } +case class GenericBundle[T <: Data, U <: Data](val a: T, val b: U) extends Bundle + +class SimpleBundleElemOrder extends Module { + val in1 = IO(Input(GenericBundle(a = Bool(), b = UInt(8.W)))) + val in2 = IO(Input(GenericBundle(b = UInt(8.W), a = Bool()))) + val out1 = IO(Output(UInt(in1.getWidth.W))) + val out2 = IO(Output(UInt(in2.getWidth.W))) + out1 := in1.asUInt + out2 := in2.asUInt +} + +class BundleElemOrder1(gen: => UInt) extends Bundle { + val a = UInt(8.W) + val b = gen + val c = UInt(8.W) + +} +class BundleElemOrder2(gen: UInt) extends Bundle { + val a = UInt(8.W) + val b = gen + val c = UInt(8.W) + +} + +class SimpleBundleElemOrderByName extends Module { + val in0 = IO(Input(new BundleElemOrder1(UInt(8.W)))) + val in1 = IO(Input(new BundleElemOrder2(UInt(8.W)))) + val out0 = IO(Output(UInt(24.W))) + val out1 = IO(Output(UInt(24.W))) + out0 := in0.asUInt + out1 := in1.asUInt +} + /* Testing whether gen fields superFoo and superQux can be found when they are * declared in a superclass * @@ -274,11 +308,11 @@ class BundleElementsSpec extends AnyFreeSpec with Matchers { "animals" -> _.animals, "baz" -> _.baz, "bar" -> _.bar, + "foo" -> _.foo, "varmint" -> _.varmint, "fieldThree" -> _.fieldThree, "fieldTwo" -> _.fieldTwo, - "fieldOne" -> _.fieldOne, - "foo" -> _.foo + "fieldOne" -> _.fieldOne ) assertElementsMatchExpected(out5)("fieldThree" -> _.fieldThree, "fieldTwo" -> _.fieldTwo, "fieldOne" -> _.fieldOne) assertElementsMatchExpected(out2)("notAbstract" -> _.notAbstract, "fromAbstractBundle" -> _.fromAbstractBundle) @@ -469,9 +503,9 @@ class BundleElementsSpec extends AnyFreeSpec with Matchers { val io = IO(new BpipOptionBundle) assertElementsMatchExpected(io)( - "bpipUIntLazyVal" -> _.bpipUIntLazyVal, "bpipOptionUInt" -> _.bpipOptionUInt.get, "bpipUIntVar" -> _.bpipUIntVar, + "bpipUIntLazyVal" -> _.bpipUIntLazyVal, "bpipUIntVal" -> _.bpipUIntVal ) } @@ -479,6 +513,16 @@ class BundleElementsSpec extends AnyFreeSpec with Matchers { ChiselStage.emitCHIRRTL(new ALU(ALUConfig(10, mul = true, b = false))) } + "Bundle elements should be ordered in order of declaration" in { + val chirrtl = ChiselStage.emitCHIRRTL(new SimpleBundleElemOrder) + chirrtl should include("input in1 : { a : UInt<1>, b : UInt<8>}") + chirrtl should include("input in2 : { a : UInt<1>, b : UInt<8>}") + + val chirrtl2 = ChiselStage.emitCHIRRTL(new SimpleBundleElemOrderByName) + chirrtl2 should include("input in0 : { a : UInt<8>, b : UInt<8>, c : UInt<8>}") + chirrtl2 should include("input in1 : { a : UInt<8>, b : UInt<8>, c : UInt<8>}") + } + "TraceSpec test, different version found in TraceSpec.scala" in { class Bundle0 extends Bundle { val a = UInt(8.W) From 6f4200a421a4c05ac4a6035b2a9cbfd71c50ddd3 Mon Sep 17 00:00:00 2001 From: Aditya Naik Date: Thu, 1 Aug 2024 11:00:17 -0700 Subject: [PATCH 5/5] Add assertElementsMatch tests --- .../chiselTests/BundleElementsSpec.scala | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/test/scala/chiselTests/BundleElementsSpec.scala b/src/test/scala/chiselTests/BundleElementsSpec.scala index 41556b41b64..5a68acab182 100644 --- a/src/test/scala/chiselTests/BundleElementsSpec.scala +++ b/src/test/scala/chiselTests/BundleElementsSpec.scala @@ -220,6 +220,12 @@ class SimpleBundleElemOrder extends Module { val out2 = IO(Output(UInt(in2.getWidth.W))) out1 := in1.asUInt out2 := in2.asUInt + + val io = IO(new GenericBundle(UInt(8.W), UInt(8.W))) + assertElementsMatchExpected(io)( + "b" -> _.b, + "a" -> _.a + ) } class BundleElemOrder1(gen: => UInt) extends Bundle { @@ -242,6 +248,20 @@ class SimpleBundleElemOrderByName extends Module { val out1 = IO(Output(UInt(24.W))) out0 := in0.asUInt out1 := in1.asUInt + + val io1 = IO(new BundleElemOrder1(UInt(8.W))) + assertElementsMatchExpected(io1)( + "c" -> _.c, + "b" -> _.b, + "a" -> _.a + ) + + val io2 = IO(new BundleElemOrder2(UInt(8.W))) + assertElementsMatchExpected(io2)( + "c" -> _.c, + "b" -> _.b, + "a" -> _.a + ) } /* Testing whether gen fields superFoo and superQux can be found when they are