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

Add recursive Bundle and generic Data equality (===) #2669

Merged
merged 9 commits into from
Aug 17, 2022
46 changes: 45 additions & 1 deletion core/src/main/scala/chisel3/Data.scala
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ package chisel3
import chisel3.experimental.dataview.reify

import scala.language.experimental.macros
import chisel3.experimental.{Analog, BaseModule, DataMirror, FixedPoint, Interval}
import chisel3.experimental.{Analog, BaseModule, DataMirror, EnumType, FixedPoint, IO, Interval}
import chisel3.internal.Builder.pushCommand
import chisel3.internal._
import chisel3.internal.firrtl._
Expand Down Expand Up @@ -893,6 +893,50 @@ abstract class Data extends HasId with NamedComponent with SourceInfoDoc {
def toPrintable: Printable
}

object Data {
implicit class DataEquality[T <: Data](lhs: T)(implicit sourceInfo: SourceInfo, compileOptions: CompileOptions) {
def ===(rhs: T): Bool = {
jared-barocsi marked this conversation as resolved.
Show resolved Hide resolved
(lhs, rhs) match {
case (thiz: UInt, that: UInt) => thiz === that
case (thiz: SInt, that: SInt) => thiz === that
case (thiz: AsyncReset, that: AsyncReset) => thiz.asBool === that.asBool
case (thiz: Reset, that: Reset) => thiz === that
case (thiz: Interval, that: Interval) => thiz === that
case (thiz: FixedPoint, that: FixedPoint) => thiz === that
case (thiz: EnumType, that: EnumType) => thiz === that
case (thiz: Clock, that: Clock) => thiz.asUInt === that.asUInt
case (thiz: Vec[_], that: Vec[_]) =>
if (thiz.length != that.length) {
false.B
jared-barocsi marked this conversation as resolved.
Show resolved Hide resolved
} else {
thiz.getElements
.zip(that.getElements)
.map { case (thisData, thatData) => thisData === thatData }
.reduce(_ && _)
}
case (thiz: Record, that: Record) =>
if (thiz.elements.size != that.elements.size) {
false.B
jared-barocsi marked this conversation as resolved.
Show resolved Hide resolved
} else {
thiz.elements
.zip(that.elements)
jared-barocsi marked this conversation as resolved.
Show resolved Hide resolved
.map {
case ((thisName, thisData), (thatName, thatData)) =>
thisName.equals(thatName).asBool && thisData === thatData
jared-barocsi marked this conversation as resolved.
Show resolved Hide resolved
}
.reduce(_ && _)
}
// This should be matching to (DontCare, DontCare) but the compiler wasn't happy with that
case (_: DontCare.type, _: DontCare.type) => true.B
Copy link
Contributor

Choose a reason for hiding this comment

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

I think comparing DontCare should not be possible. We do not want these two versions of code to (silently) have different behavior:

DontCare === DontCare
val a = Wire(UInt(4.W))
val b = Wire(UInt(4.W))
a := DontCare
b := DontCare
a === b

Copy link
Contributor

Choose a reason for hiding this comment

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

We do not want these two versions of code to (silently) have different behavior

Absolutely agreed, but the behavior of the two actually is the same, both will give true. Here's a Scastie illustrating the behavior:

import chisel3._
import chisel3.util.Valid
import chisel3.experimental.BundleLiterals._

class Example extends Module {

  val out1, out2      = IO(Output(Bool()))

  val u1 = WireInit(UInt(8.W), DontCare)
  val u2 = WireInit(UInt(8.W), DontCare)
  out1 := u1 === u2
  
  val b1 = Valid(UInt(8.W)).Lit(_.bits -> 123.U) // unspecified is implicitly DontCare
  val b2 = Valid(UInt(8.W)).Lit(_.bits -> 123.U)
  out2 := b1.asUInt === b2.asUInt
}

This gives the following FIRRTL

circuit Example :
  module Example :
    input clock : Clock
    input reset : UInt<1>
    output out1 : UInt<1>
    output out2 : UInt<1>

    wire u1 : UInt<8>
    u1 is invalid
    wire u2 : UInt<8>
    u2 is invalid
    node _out1_T = eq(u1, u2)
    out1 <= _out1_T
    wire _out2_WIRE : UInt<1>
    _out2_WIRE is invalid
    node _out2_T = cat(_out2_WIRE, UInt<7>("h7b"))
    wire _out2_WIRE_1 : UInt<1>
    _out2_WIRE_1 is invalid
    node _out2_T_1 = cat(_out2_WIRE_1, UInt<7>("h7b"))
    node _out2_T_2 = eq(_out2_T, _out2_T_1)
    out2 <= _out2_T_2

And the following Verilog:

module Example(
  input   clock,
  input   reset,
  output  out1,
  output  out2
);
  assign out1 = 1'h1; // @[main.scala 13:14]
  assign out2 = 1'h1; // @[main.scala 17:21]
endmodule

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure this is a good thing, but it is consistent.

Copy link
Member

Choose a reason for hiding this comment

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

As annoying as it is, comparing two invalidated wires should return true. A lot of Chisel code intentionally or unintentionally assumes that invalid is zero in certain contexts. This is documented in CIRCT. I would love to change this, but it would be very difficult at this point. I do think defining a new concept of poison and seeing where that goes is tractable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do think defining a new concept of poison and seeing where that goes is tractable.

But beyond the scope of this PR for sure 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

The ambiguity around DontCare === 0.U is why I feel like making them uncomparable by generating an elaboration time error might be the best solution here.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that the ambiguity is a problem, but is it a better outcome that the following would fail:

val b1 = Valid(UInt(8.W)).Lit(_.bits -> 123.U) // unspecified is implicitly DontCare
val b2 = Valid(UInt(8.W)).Lit(_.bits -> 123.U)
b1 === b2

But this would not?

val b1 = Valid(UInt(8.W)).Lit(_.bits -> 123.U) // unspecified is implicitly DontCare
val b2 = Valid(UInt(8.W)).Lit(_.bits -> 123.U)
WireInit(b1) === WireInit(b2)


case (_: Analog, _: Analog) => throwException("Equality isn't defined for Analog values")
// Runtime types are different
case (_, _) => false.B
jared-barocsi marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
}

trait WireFactory {

/** Construct a [[Wire]] from a type template
Expand Down
216 changes: 216 additions & 0 deletions src/test/scala/chiselTests/DataEqualitySpec.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,216 @@
package chiselTests

import chisel3._
import chisel3.experimental.VecLiterals._
import chisel3.experimental.BundleLiterals._
import chisel3.experimental.{Analog, ChiselEnum, ChiselRange, FixedPoint, Interval}
import chisel3.stage.ChiselStage
import chisel3.testers.BasicTester

class EqualityTester(lhsGen: => Data, rhsGen: => Data) extends BasicTester {
val lhs = lhsGen
val rhs = rhsGen
assert(lhs === rhs)

stop()
}

class AnalogBundle extends Bundle {
val analog = Analog(32.W)
}

class AnalogExceptionModule extends Module {
class AnalogExceptionModuleIO extends Bundle {
val bundle1 = new AnalogBundle
val bundle2 = new AnalogBundle
}

val io = IO(new AnalogExceptionModuleIO)
}

class AnalogExceptionTester extends BasicTester {
val module = Module(new AnalogExceptionModule)

module.io.bundle1 <> DontCare
module.io.bundle2

assert(module.io.bundle1 === module.io.bundle2)

stop()
}

class DataEqualitySpec extends ChiselFlatSpec with Utils {
object MyEnum extends ChiselEnum {
val sA, sB = Value
}
object MyEnumB extends ChiselEnum {
val sA, sB = Value
}
class MyBundle extends Bundle {
val a = UInt(8.W)
val b = Bool()
val c = MyEnum()
}

class LongBundle extends Bundle {
val a = UInt(48.W)
val b = SInt(32.W)
val c = FixedPoint(16.W, 4.BP)
}

behavior.of("UInt === UInt")
it should "pass with equal values" in {
assertTesterPasses {
new EqualityTester(0.U, 0.U)
}
}
it should "fail with differing values" in {
assertTesterFails {
new EqualityTester(0.U, 1.U)
}
}

behavior.of("SInt === SInt")
it should "pass with equal values" in {
assertTesterPasses {
new EqualityTester(0.S, 0.S)
}
}
it should "fail with differing values" in {
assertTesterFails {
new EqualityTester(0.S, 1.S)
}
}

behavior.of("Reset === Reset")
it should "pass with equal values" in {
assertTesterPasses {
new EqualityTester(true.B, true.B)
}
}
it should "fail with differing values" in {
assertTesterFails {
new EqualityTester(true.B, false.B)
}
}

behavior.of("AsyncReset === AsyncReset")
it should "pass with equal values" in {
assertTesterPasses {
new EqualityTester(true.B.asAsyncReset, true.B.asAsyncReset)
}
}
it should "fail with differing values" in {
assertTesterFails {
new EqualityTester(true.B.asAsyncReset, false.B.asAsyncReset)
}
}

behavior.of("Interval === Interval")
it should "pass with equal values" in {
assertTesterPasses {
new EqualityTester(2.I, 2.I)
}
}
it should "fail with differing values" in {
assertTesterFails {
new EqualityTester(2.I, 3.I)
}
}

behavior.of("FixedPoint === FixedPoint")
it should "pass with equal values" in {
assertTesterPasses {
new EqualityTester(4.5.F(16.W, 4.BP), 4.5.F(16.W, 4.BP))
}
}
it should "fail with differing values" in {
assertTesterFails {
new EqualityTester(4.5.F(16.W, 4.BP), 4.6.F(16.W, 4.BP))
}
}

behavior.of("ChiselEnum === ChiselEnum")
it should "pass with equal values" in {
assertTesterPasses {
new EqualityTester(MyEnum.sA, MyEnum.sA)
}
}
it should "fail with differing values" in {
assertTesterFails {
new EqualityTester(MyEnum.sA, MyEnum.sB)
}
}

behavior.of("Vec === Vec")
it should "pass with equal sizes, equal values" in {
assertTesterPasses {
new EqualityTester(
Vec(3, UInt(8.W)).Lit(0 -> 1.U, 1 -> 2.U, 2 -> 3.U),
Vec(3, UInt(8.W)).Lit(0 -> 1.U, 1 -> 2.U, 2 -> 3.U)
)
}
}
it should "fail with equal sizes, differing values" in {
assertTesterFails {
new EqualityTester(
Vec(3, UInt(8.W)).Lit(0 -> 1.U, 1 -> 2.U, 2 -> 3.U),
Vec(3, UInt(8.W)).Lit(0 -> 0.U, 1 -> 1.U, 2 -> 2.U)
)
}
}
it should "fail with differing sizes" in {
assertTesterFails {
new EqualityTester(
Vec(3, UInt(8.W)).Lit(0 -> 1.U, 1 -> 2.U, 2 -> 3.U),
Vec(4, UInt(8.W)).Lit(0 -> 1.U, 1 -> 2.U, 2 -> 3.U, 3 -> 4.U)
)
}
}

behavior.of("Bundle === Bundle")
it should "pass with equal type, equal values" in {
assertTesterPasses {
new EqualityTester(
(new MyBundle).Lit(_.a -> 42.U, _.b -> false.B, _.c -> MyEnum.sB),
(new MyBundle).Lit(_.a -> 42.U, _.b -> false.B, _.c -> MyEnum.sB)
)
}
}
it should "fail with equal type, differing values" in {
assertTesterFails {
new EqualityTester(
(new MyBundle).Lit(_.a -> 42.U, _.b -> false.B, _.c -> MyEnum.sB),
(new MyBundle).Lit(_.a -> 42.U, _.b -> false.B, _.c -> MyEnum.sA)
)
}
}

behavior.of("DontCare")
it should "only be equal to itself" in {
assertTesterPasses {
new EqualityTester(DontCare, DontCare)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think two different calls to DontCare should be equal. That is not what happens in the firrtl if you did not check in the frontend.

}
}
it should "not be equal to anything else" in {
assertTesterFails {
new EqualityTester(DontCare, 0.U)
}
assertTesterFails {
new EqualityTester(1.S, DontCare)
}
assertTesterFails {
new EqualityTester(DontCare, true.B)
}
assertTesterFails {
new EqualityTester(false.B.asAsyncReset, DontCare)
}
}

behavior.of("Analog === Analog")
it should "throw a ChiselException" in {
(the[ChiselException] thrownBy extractCause[ChiselException] {
assertTesterFails { new AnalogExceptionTester }
}).getMessage should include("Equality isn't defined for Analog values")
}
}