Fixed-Point type deprecation

[mvanbeirendonck]

Type of issue: Feature Request

Hi! I'm filing this issue about the deprecation of the Fixed-Point type. I believe this is a specific deprecation that falls inside the following category:

For users who depend on deprecated features for which there is no obvious replacement, please reach out to the Chisel developers by filing an issue: https://github.com/chipsalliance/chisel3/issues.

I am a researcher working on hardware acceleration of cryptography. We heavily used Chisel and the Fixed-Point type for a recent FPGA accelerator we built called FPT. We aim to maintain and further develop this accelerator. We want to be able to move to updated Chisel versions in the future, hence why I am reaching out.

I understand that Fixed-Point is a niche and not-so-utilized feature. Are there any plans to add support for Fixed-Point within the new MLIR FIRRTL compiler? If not, is this a "good first issue" we can tackle? FPT was our first project using Chisel and Scala, so we are not that experienced yet.

Another possibility we looked into is to rebuild the Fixed-Point type fully within Chisel, on top of the SInt and UInt types. What is your opinion on that? Why is support preferred or necessary at the FIRRTL level?

I would appreciate any feedback!

Thanks,

Is your feature request related to a problem? Please describe.
Deprecation of Fixed-Point type.

Describe the solution you'd like
Feedback on best way to migrate a Fixed-Point design.

Describe alternatives you've considered

• Re-integrating the Fixed-Point type in the MLIR FIRRTL compiler
• Building a Fixed-Point type entirely in Chisel on top of SInt/UInt

Additional context

What is the use case for implementing this feature?
Being able to migrate a Fixed-Point design to future Chisel versions.

[seldridge]

Thanks for reaching out.

Adding Fixed Point back to Chisel as either libraries that wrap UInt/SInt or as something more clever would work.

We don't want to support these (or Interval types) natively in FIRRTL with as these were never widely used (though did have users) and they are better represented as true user-defined types.

An example of how this could work is something like. This uses Bundle as that is a type that a user is allowed to extend. Doing this inside Chisel would allow this to directly extend Data and could avoid being a Bundle. There may be a way to use an OpaqueType to use a Record to avoid having to extend Data:

class FixedPoint(val binaryPoint: Int, val dataWidth: Int) extends Bundle with Num[FixedPoint] {
  val data = UInt(dataWidth.W)

  override def do_+(that: FixedPoint)(implicit sourceInfo: SourceInfo): FixedPoint = {
    // This should align the binary points and not error!
    require(binaryPoint == that.binaryPoint)

    // This should do some implicit padding/extension and not error!
    require(dataWidth == that.dataWidth)

    val _sum = Wire(new FixedPoint(binaryPoint, dataWidth))
    _sum.data := data + this.data
    _sum
  }

  // Implement all these methods!
  override def do_-(that: FixedPoint)(implicit sourceInfo: SourceInfo): FixedPoint = ???
  override def do_*(that: FixedPoint)(implicit sourceInfo: SourceInfo): FixedPoint = ???
  override def do_/(that: FixedPoint)(implicit sourceInfo: SourceInfo): FixedPoint = ???
  override def do_%(that: FixedPoint)(implicit sourceInfo: SourceInfo): FixedPoint = ???
  override def do_<(that: FixedPoint)(implicit sourceInfo: SourceInfo): Bool = ???
  override def do_<=(that: FixedPoint)(implicit sourceInfo: SourceInfo): Bool = ???
  override def do_>(that: FixedPoint)(implicit sourceInfo: SourceInfo): Bool = ???
  override def do_>=(that: FixedPoint)(implicit sourceInfo: SourceInfo): Bool = ???
  override def do_abs(implicit sourceInfo: SourceInfo) = ???
}

class Foo extends Module {
  val a, b = IO(Input(new FixedPoint(4, 8)))
  val c = IO(Output(new FixedPoint(4, 8)))

  c := a + b
}

object Main extends App {
  println(
    ChiselStage.emitSystemVerilog(
      gen = new Foo,
      firtoolOpts = Array("-disable-all-randomization", "-strip-debug-info")
    )
  )
}

I spoke to @jackkoenig about this and he suggested using a test-driven approach where you use something like this snippet (https://scastie.scala-lang.org/3Kqw0Q9mShGkUW8ZjA0zdQ) to test the behavior of the old SFC implementation against whatever Chisel-native Fixed Point type is added.

[mvanbeirendonck]

Thanks for providing some guidance! I already played around a bit with your suggestion in the past week.

On our side, it actually looks most interesting to have FixedPoint as a user-defined Bundle or Record, or at least as something that we can still extend later on (so not a sealed class like UInt or SInt). The reason is that we require some more exotic functionality, e.g. explicitly allowing overflows without padding in a signal assignment. This probably shouldn't be in a base FixedPoint class, hence why we want it to be extensible. If we would implement a user-defined type extending Bundle or Record, what would be the best way to contribute it?

Regarding the test-driven approach you suggest, what would be the exact "goal"? To have the emitted FIRRTL be equal to the MLIR FIRRTL (up to the Fixed type annotations)?

[seldridge]

Regarding the test-driven approach you suggest, what would be the exact "goal"? To have the emitted FIRRTL be equal to the MLIR FIRRTL (up to the Fixed type annotations)?

The Scastie example doesn't show it. However, it would be ideal if the emitted Verilog was the same with the SFC and with the new Chisel-only Fixed Point types. Here is an updated Scastie which is just printing the Verilog out: https://scastie.scala-lang.org/6dogAWcSRhed7QmmlUuD8w

If we would implement a user-defined type extending Bundle or Record, what would be the best way to contribute it?

If the exotic functionality seems to make sense, I wouldn't be against adding it. If it's totally funky (or something that you wouldn't want to contribute), then you could probably do it by landing the FixedPoint support upstream in Chisel and using extension methods (implicit classes) to add only the weirder ops to even sealed/final classes.

Roughly:

final class FixedPoint

object FunkyStuff {
  implicit class FixedPointHelpers(a: FixedPoint) {
    def foo(that: a): FixedPoint = ???
  }
}

[konda-x1]

Hi. My colleagues and I also have an interest in FixedPoint as we use it as a dependency in our designs. I'm also interested in doing development work to ensure it lives on in the future, so we can eventually migrate our code base to newer Chisel versions without worries. Thanks @seldridge for providing guidance in this direction!

Adding Fixed Point back to Chisel as either libraries that wrap UInt/SInt or as something more clever would work.

However, it would be ideal if the emitted Verilog was the same with the SFC and with the new Chisel-only Fixed Point types.

Out of curiosity, what would a "more clever" solution look like? Is there a possibility of a non "Chisel-only" solution? I'm thinking of something that wouldn't depend on UInt/SInt, but would rather be on equal footing with those types, just like FixedPoint was before being deprecated. Sorry, I'm not too familiar with how this all works, but I want to get a sense of just what all the possibilities are for implementation, ie. if there are alternatives to the approach of extending Bundle and wrapping UInt/SInt. I've seen the "Chisel breakdown 3" talk and now I understand that you use Chisel IR as a first step in elaboration, but I don't really know how exactly you go from that to FIRRTL (or MLIR?) and if following an approach like that is something you can actually do when writing a library that works with Chisel to add a new Chisel type. Thanks.

[seldridge]

More clever would push in the direction of supporting user-defined types in FIRRTL and exposing that to Chisel. Users would then be free to define any type they need and the operations it supports. The new type alias feature (chipsalliance/firrtl-spec#106) is the first piece of this. (Admittedly, for a different use---associating names with bundles to improve Verilog emission.) However, we are a ways away from user-defined primitive operations (functions?) in FIRRTL, though. 😅

The primary motivation of the Bundle approach is that it works today without any modifications to Chisel internals while providing almost the same behavior as before (except binary point inference). Allowing Chisel users to extend Data directly would be another option.

When a Chisel program executes, it's building up "Chisel IR" operations that match almost exactly the order of execution of the Chisel program. That Chisel IR is converted to FIRRTL IR with essentially no modifications. That is then written to FIRRTL text and given to a FIRRTL compiler to convert to Verilog. That FIRRTL compiler(CIRCT) is based on MLIR and will parse the FIRRTL text into FIRRTL Dialect (which is an MLIR representation of FIRRTL). That eventually becomes Verilog after going through mandatory passes at the FIRRTL Dialect level and after being converted to other, lower-level dialects.

It is good to keep Chisel IR as essentially the same as FIRRTL IR without introducing any processing between them. E.g., it would be non-ideal to add Chisel IR operations to represent fixed point and then convert them to FIRRTL IR UInt/SInt. This would start to look like a Scala compiler between Chisel and CIRCT which we'd like to avoid.

[konda-x1]

Hi. I gave it a shot at implementing a FixedPoint library. The goal was to replicate the existing FixedPoint API as faithfully as possible so existing code that relied on FixedPoint can continue to work properly with little to no changes. I stole Chisel's FixedPoint tests and it seems to be working fine with those. I also integrated it by hand into dsptools in order to test it with its test suite as well. Those tests seem to pass as well, barring 3 tests which fail due to reasons out of my control. For pretty much every FixedPoint feature there is, I did a side-by-side comparison of generated SystemVerilog for Chisel's FixedPoint and my FixedPoint, and they seem to be functionally equivalent in all cases. My FixedPoint generates a few more wires and the naming can get a little weird but it's the same code nonetheless. You can find my implementation here: https://github.com/ucb-bar/fixedpoint

Unfortunately, there were some limitations that were hard to overcome, so here's a few remarks from my experience of attempting to get this to work:

• I can't access or modify cloneSuperType behavior, which I needed to align FixedPoints by differernt binary points when creating a Vec or Mux. In the end I created my own version of each Mux object which first does this alignment of FixedPoints and then calls the corresponding Chisel Mux object. And shadowing the corresponding Chisel Vec objects has yet to be done...

• I also needed to align FixedPoints by binary point before connecting them together. I managed to pull this off by overriding connect and bulkConnect to call := and <> on each bundle's data fields respectively, after doing some bit shift operations. I also implemented some sort of binary point inference inside the connect methods.

  Unfortunately, these methods are not called when a FixedPoint is inside a Bundle or Vec, so they won't be connected properly. I created a trait ForceElementwiseConnect for Records which overrides the connect and bulkConnect methods to call := and <> on each of the record fields respectively. That way I could access FixedPoint's overridden connect behavior. But this isn't an ideal or clean solution, and in the case of connecting together Vecs of FixedPoints with different binary points I don't know what to do.

• Mux1H doesn't work with Aggregates with inferred widths, so you can't use this library's FixedPoint with them without specifying widths, which you didn't need to do with Chisel's FixedPoint.

So, that's the result of my efforts. I don't know how things should proceed from here regarding FixedPoint...

[jackkoenig]

That's some nice work @konda-x1. I think on the Chisel side we should help you lift some of those limitations, thank you for laying them out and explaining them here. We had been resistant to allowing overriding connection behavior--I didn't realize it was even possible for the methods where it was possible--so perhaps we can come up with a better API to expose for you to use here. Same thing with cloneSuperType.

[jurevreca12]

Hey,
I am also a researcher. I am working on generating highly quantized neural network implementations on FPGA (https://github.com/cs-jsi/chisel4ml). I would also be very interested to see the FixedPoint type be available in some form. I am also willing to help with the implementation and testing of any proposed solution.

[jurevreca12]

One thing I forgot to add. I have one issue with the original FixedPoint implementation in Chisel, and that is that that it assumes a signed number. Having unsigned fixed-point numbers would also be nice.

[konda-x1]

Thanks, @jackkoenig. There is one more tiny limitation which I forgot about, and that is the inability to access the stringAccessor method when implementing the toString method. I needed this particularly in the case where my bundle is connected to DontCare, so I can add that info like the other Chisel types do. I ended up using the toString method of the underlying SInt field and then extracting the needed information via regex. Simply overriding className instead of toString for this purpose is inappropriate because toString will then also return the underlying SInt content of the Bundle.

If you guys need a hand in providing APIs that will enable the fixed-point library to overcome its current limitations, I'm willing to help. I've done my fair share of debugging during the development of this library, so I can roughly tell what goes on under the hood when the limitations are hit. My naive guess about what needs to be provided in the API would be the following:

• In the case of cloneSuperType:

  • Inside Aggregate or Record (or rather, inside a new trait that the user can extend their type with), provide the following method that the user can override:

    def cloneSuperType(that: this.type): this.type

  • Inside the else clause (or add a new else-if clause which checks if the new trait is inherited from) of object cloneSuperType's apply method—when filteredElts.head is not an instance of Bits—call something like head.cloneSuperType(elt) with filteredElts.reduce.
  • After the super type has been determined, another method would need to be provided for implementation, that would adjust individual instances to the super type. This is where I would, in the case of fixed-points, add the bit-shift operations on the underlying SInt fields to align them to the super type's binary point.

• In the case of connect behavior:

  • Inside Record or a new trait, provide a method that will be executed just before a firrtl (or is it chirrtl?) Connect operation is pushed:

    def beforeConnect(that: Data): Unit

  • Inside MonoConnect.connect, where case (sink_r: Record, source_r: Record) is being handled, just before the call to pushCommand(Connect(...)), execute the previously defined beforeConnect method wherever necessary. This would need to be done recursively, because a normal bundle may not need to implement beforeConnect, but it might—as one of its elements—contain a bundle which does implement this method. I guess each bundle could maintain a list of elements where this recursion is necessary. Or maybe the top-level bundle—or every bundle at each level of nesting—would have a flattened list of all (sub)elements that need to run the beforeConnect method. Connection to DontCare is handled as a separate case, so maybe it might not be a bad idea to call beforeConnect there too, for the sake of completeness. Or maybe not; I'm not really sure.

• As for stringAccessor, I think I would just change its scope from private[chisel3] to protected.

[konda-x1]

I agree with @jurevreca12 that having unsigned fixed-points would be convenient. It also shouldn't be hard to implement them, now that the signed version is more or less fleshed out. I'm interested in what the Chisel developers think of this, and, if they also agree with having an unsigned fixed-point type, I would like to consult with them about the best way to provide an API for both signed and unsigned fixed-points.

I think it goes without saying that the fixed-point type would at least have a base class that takes a generic parameter, which could either be SInt or UInt. However, providing a class like that as part of the public API would, in my opinion, make things unnecessarily complicated for the users, so I would make the generic base class—let's call it FixedPointBase—private, and extend it with concrete implementations that are based on SInt/UInt:

class SFixedPoint extends FixedPointBase[SInt]
class UFixedPoint extends FixedPointBase[UInt]

This way users wouldn't have to deal with generic parameters, especially since there are only two sensible options to choose from. If the demand arises to make FixedPointBase non-private so users can extend it with their own base types—a sign-magnitude signed-int implementation comes to mind—then we could eventually make it public, but it would definitely start out as a package-private class of the fixed-point library.

The choice in naming for SFixedPoint/UFixedPoint is intended to mimic the naming of SInt/UInt. In that case the type FixedPoint would cease to exist, and existing usages of "FixedPoint" would simply need to be replaced with "SFixedPoint".