Reintroduce FixedPoint support

[mvanbeirendonck]

https://github.com/ucb-bar/chiseltest/pull/624 removed FixedPoint support because this feature was removed from Chisel5.

There is some effort to reintroduce FixedPoint support in Chisel (https://github.com/chipsalliance/chisel/issues/3161) through a custom library (https://github.com/ucb-bar/fixedpoint).

This pull request reintroduces FixedPoint support in Chiseltest5 using that fixedpoint library.

---

The main change in this PR are:

• Added fixedpoint as a git submodule
• Cherry-picked https://github.com/ucb-bar/chiseltest/pull/687 because that allows easy registering of fixedpoint as a subproject
• git revert d8a44b2

For this last one, git revert d8a44b2, I only reintroduced changes related to FixedPoint, not Interval.

I have also not included any old FixedPoint support that was present in the legacy PeekPokeTester. The reason for this is that using FixedPoint with PeekPokeTester was already integrated by @konda-x1 and @milovanovic into dsptools (https://github.com/ucb-bar/dsptools/blob/master/src/main/scala/dsptools/misc/PeekPokeDspExtensions.scala).

---

This is my first pull request for a Scala project, so let me know if there is anything you'd like me to improve :-)

[CLAassistant]

All committers have signed the CLA.

[ekiwi]

Thanks for working on this. I will do a detailed review later. Currently the major thing you will have to fix is that I do not want to add the fixed point library as a submodule. Instead, chiseltest needs to depend on a published version of this library, just as it does with chisel. Otherwise, publishing chiseltest itself will become too complicated.

As an alternative to depending on a published version of the fixed point library, we could also investigate what it would take to change chiseltest in such a way that the Fixed point support could be implemented as part of the fixed point library. That way the fixed point library would depend on chiseltest instead of chiseltest depending on the fixed point library.

[mvanbeirendonck]

Thanks for the quick reply! I agree that fixedpoint should not be a submodule here. I took the submodule approach as in https://github.com/ucb-bar/dsptools to get a quick proof-of-concept that everything works.

I will ask whether the fixedpoint library is mature enough to be published and in the meantime also look into your second alternative.

[mvanbeirendonck]

As an alternative to depending on a published version of the fixed point library, we could also investigate what it would take to change chiseltest in such a way that the Fixed point support could be implemented as part of the fixed point library. That way the fixed point library would depend on chiseltest instead of chiseltest depending on the fixed point library.

It seems very feasible to move testableFixedPoint over from chiseltest to fixedpoint with virtually no changes: https://github.com/mvanbeirendonck/fixedpoint/tree/chiseltest-support.

A few remarks:

• I had to also copy over Utils as it is a private object.
• There is an error with the tests that intercept[exceptions.TestFailedException] in FaultDecoderTest.

This last one fails with:

Expected exception org.scalatest.exceptions.TestFailedException to be thrown, but java.lang.IllegalArgumentException was thrown

If I remove the intercept:

[info] java.base/java.lang.Thread.run(Thread.java:829)
[info]   at scala.Predef$.require(Predef.scala:337)
[info]   at chiseltest.internal.TestEnvInterface.signalExpectFailure(TestEnvInterface.scala:54)
[info]   at chiseltest.internal.TestEnvInterface.signalExpectFailure$(TestEnvInterface.scala:45)
[info]   at FixedPointTests.signalExpectFailure(FixedPointTests.scala:7)
[info]   at fixedpoint.package$Utils$.expectFailed(package.scala:217)
[info]   at fixedpoint.package$Utils$.expectEpsilon(package.scala:169)
[info]   at fixedpoint.package$testableFixedPoint.expectInternal(package.scala:41)
[info]   at fixedpoint.package$testableFixedPoint.expect(package.scala:43)
[info]   at FixedPointTests$$anonfun$FixedPointTests$$testFixedPointDivide$1.apply(FixedPointTests.scala:42)
[info]   at FixedPointTests$$anonfun$FixedPointTests$$testFixedPointDivide$1.apply(FixedPointTests.scala:38)

I'm not entirely sure, but would think the problem is with:

    val expectStackDepth = trace.getStackTrace.indexWhere(ste =>
      ste.getClassName.startsWith(
        "chiseltest.package$"
      ) && (ste.getMethodName == "expect" || ste.getMethodName == "expectPartial")
    )

We need somehow to pass fixedpoint.package instead of chiseltest.package?

[ekiwi]

I had to also copy over Utils as it is a private object.

I think we will be able to make that more public, such that you can use it without copying.

There is an error with the tests that intercept[exceptions.TestFailedException] in FaultDecoderTest.

Could you see if there is an easy way to make this list that it is checking against extensible? Or maybe even just confirm that adding your package to the check makes everything else work. Then we could try together to allow an external package to add itself.

[mvanbeirendonck]

I had to also copy over Utils as it is a private object.

I think we will be able to make that more public, such that you can use it without copying.

There is an error with the tests that intercept[exceptions.TestFailedException] in FaultDecoderTest.

Could you see if there is an easy way to make this list that it is checking against extensible? Or maybe even just confirm that adding your package to the check makes everything else work. Then we could try together to allow an external package to add itself.

I created another branch of chiseltest here https://github.com/mvanbeirendonck/chiseltest/tree/fixedpoint-dependency that does exactly this. It is now a dependency for fixedpoint here https://github.com/mvanbeirendonck/fixedpoint/tree/chiseltest-support.

1. I opened up a few methods from chiseltest.Utils.

2. I removed ste.getClassName.startsWith("chiseltest.package$"), which indeed fixes the thrown errors.

As you mentioned, I can also make this last check extensible for other package names. However, I was wondering if that is really necessary. What does this check avoid? If there is a method called expect that is calling chiseltest's expect, the chiseltest one is still the one that would get found by indexWhere(ste => (ste.getMethodName == "expect" || ste.getMethodName == "expectPartial"))?

[mvanbeirendonck]

On another note, in a project of mine, we created some extensions to chiseltest that allow things like peeking a Vec[Vec[UInt]] with a Seq[Seq[BigInt]] in a generic way using typeclasses.

This looks roughly like this:

object ChiselTestUtilities {

  trait Pokeable[A <: Data, B] {
    def poke(data: A, value: B): Unit
  }

  trait Expectable[A <: Data, B] {
    def expect(data: A, value: B): Unit
  }

  trait ExpectableEpsilon[A <: Data, B] {
    def expect(data: A, value: B, epsilon: Double): Unit
  }
  
  // Implicit class for extension method syntax
  implicit class PeekPokeableOps[A <: Data](data: A) {

    def poke[B](value: B)(implicit p: Pokeable[A, B]) = p.poke(data, value)

    def expect[B](value: B)(implicit p: Expectable[A, B]) = p.expect(data, value)

    def expect[B](value: B, epsilon: Double)(implicit p: ExpectableEpsilon[A, B]) = p.expect(data, value, epsilon)
  }
    
  // Poking a Vec[A] with Iterable[B] needs evidence of a Pokeable[A, B]
  // Here we redefine all native chiselTest pokes as Pokeable instances

  implicit val pokeUIntWithUInt: Pokeable[UInt, UInt] =
    (data: UInt, value: UInt) => chiseltest.testableUInt(data).poke(value)

  implicit val pokeUIntWithInt: Pokeable[UInt, Int] =
    (data: UInt, value: Int) => chiseltest.testableUInt(data).poke(value)

  implicit val pokeUIntWithLong: Pokeable[UInt, Long] =
    (data: UInt, value: Long) => chiseltest.testableUInt(data).poke(value)

  implicit val pokeUIntWithBigInt: Pokeable[UInt, BigInt] =
    (data: UInt, value: BigInt) => chiseltest.testableUInt(data).poke(value)
    
  // We can also define new Pokeable instances that are not native in chiseltest
  
  implicit val pokeDspComplexWithComplex: Pokeable[DspComplex[FixedPoint], Complex] =
    (data: DspComplex[FixedPoint], value: Complex) => { data.real.poke(value.real); data.imag.poke(value.imag) }
    
  // Now we can have a generic method for poking a `Vec[Data]` with a `Iterable[B]`
  
  implicit def pokeVecWithIterable[A <: Data, B, C[B] <: Iterable[B]](implicit p: Pokeable[A, B]): Pokeable[Vec[A], C[B]] (data: Vec[A], value: C[B]) => {
      require(data.length == value.size, s"Cannot poke vec of length=${data.length} with iterable of length=${value.size})")
      data.lazyZip(value).foreach(p.poke)
    }

    
  }
 

If you have any interest for me to include something like this in chiseltest, I would do that before resolving this PR.

If functionality like this would be included, then supporting peek/poke for FixedPoint would only require implementing the relevant Pokeable traits etc.

[ekiwi]

If there is a method called expect that is calling chiseltest's expect, the chiseltest one is still the one that would get found by indexWhere(ste => (ste.getMethodName == "expect" || ste.getMethodName == "expectPartial"))?

Why do you think so? Wouldn't it get the user expect method?

Maybe you could add a test to demonstrate that it works.

[ekiwi]

If you have any interest for me to include something like this in chiseltest, I would do that before resolving this PR.

Yeah, I think this would be pretty cool. Happy to review the PR.

[ekiwi]

1. I opened up a few methods from chiseltest.Utils.

Some of those methods seem to be only package private. Have you considered putting your new code under a chiseltest.fixedpoint package, while keeping it as part of the fixedpoint library?

[ekiwi]

@mvanbeirendonck Do you want to make a PR with the changes that we discussed above? Any way I can help?

[mvanbeirendonck]

@mvanbeirendonck Do you want to make a PR with the changes that we discussed above? Any way I can help?

Yes! I need to find some time to give it a try, hopefully next week. I will first create a draft issue with the proposed changes, and from there identify where extra help could be useful.