Skip to content
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

Bundle elements ordering fix #4226

Merged
merged 5 commits into from
Aug 1, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions core/src/main/scala/chisel3/Aggregate.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1493,9 +1493,7 @@ 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))
}: _*)
}.reverse: _*)
}

/** This method is implemented by the compiler plugin
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Expand Down
74 changes: 69 additions & 5 deletions src/test/scala/chiselTests/BundleElementsSpec.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -210,6 +211,59 @@ 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

val io = IO(new GenericBundle(UInt(8.W), UInt(8.W)))
assertElementsMatchExpected(io)(
"b" -> _.b,
"a" -> _.a
)
}

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

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
* declared in a superclass
*
Expand Down Expand Up @@ -274,11 +328,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)
Expand Down Expand Up @@ -469,16 +523,26 @@ class BundleElementsSpec extends AnyFreeSpec with Matchers {

val io = IO(new BpipOptionBundle)
assertElementsMatchExpected(io)(
"bpipUIntLazyVal" -> _.bpipUIntLazyVal,
"bpipOptionUInt" -> _.bpipOptionUInt.get,
"bpipUIntVar" -> _.bpipUIntVar,
"bpipUIntLazyVal" -> _.bpipUIntLazyVal,
"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>}")
}

Comment on lines +536 to +545
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are good, but it would also be nice to do some assertElementsMatchExpected as that's the thing that changed (i.e. that you fixed) in this PR.

"TraceSpec test, different version found in TraceSpec.scala" in {
class Bundle0 extends Bundle {
val a = UInt(8.W)
Expand Down