Seriously questioning whether typeclasses are the right way to do things

[shunshou]

Re-emphasizing Chick's problem with having

val a = UInt(3.W) val b = 2.U * a - a

NOT using the "-" behavior defined by the typeclass.

---

All of my control logic in Chisel2 ChiselDSP used +, -, * op overrides for UInt, but this was done by explicitly using DSPUInt instead of UInt.

In order to write any control logic using the DspContext behavior for +, -, etc. I NEED to pass in gen's. This is not really tractable and also EXTREMELY confusing to the user.

Reflecting on the use-case, it seems like Typeclasses should NOT be used to override the default behaviors of things using them (only add ops, trackers, etc. on top of them).

@grebe @chick

I think this is a serious flaw, and basically I'm stuck. The API needs to be rethought out, which unfortunately requires updating all of the code yet again, but I guess that won't happen until after tapeout...

Or I just don't make tapeout.

[shunshou]

@chick I think I mentioned this in my what-should-be-tested list but, see:

def intPart(a: UInt): SInt = Cat(false.B, a).asSInt

If you have val x = Uint(3.W) and then val c = x.intPart

I assume the output is not something that has an associated TypeClass...

Which means that if you do a sub with c, DspContext won't work.

Is that correct? b/c if so, that's also extremely problematic.

In summary: Test a sequence of ops to see if the last op still gets the right DspContext. Probably a chain of add's works, but my guess is that these ConvertableTo (or From... not really sure) ops don't work...

[shunshou]

Another question: If you use Chisel cloneType on a gen, does the output also lose the gen Typeclass context?

[shunshou]

Ok, after a long conversation w/ Stephen, I think I've been convinced that you should definitely not be overriding basic ops. If you want some fancyadd with new features, it should be called fancyadd to disambiguate things. This would prevent a lot of confusion.

With the old Chisel2 DSP stuff, I made DSPUInt out of necessity (due to it being a pain to extend Chisel stuff), but it also served as a good reminder that DSPUInt had different behavior than UInt. Even then, I probably shouldn't have overloaded the meaning of + (to include + but registers, etc.) and should have had another "op" called registeredAdd to make people fully aware of what's going on. I don't know how other people feel about this, but the whole API should be carefully rethought out.

Basically, there's no point being fancy if it doesn't buy you much and just confuses people.

[shunshou]

Also, cloneType is probably ok, but I think all my convertableFrom's will result in things that aren't Real/RealBits (without changing to intPart[B <: Data:RealBits](a: T): B or something. This is kind of a big problem, and we need to take inventory on possible other ops that fail to keep typeclass info.

[shunshou]

Actually, if that's the case, maybe UInt shouldn't be something you can TypeClass and you should only allow it on Fixed and DspReal (as in the old ChiselDSP). These don't exist in "basic" Chisel (er I guess Fixed does... but it's still Dsp-specific), somaybe it's more "ok" to do non-standard things?

[shunshou]

Sorry, I keep misunderstanding. Having thte output type of ConvertableFrom directly by SInt, etc. is fine, again under the premise that TypeClasses only add features, rather than overriding them. If you import dsptools.numbers.implicits, SInt will have access to stuff like intPart, div2, etc. It's just that it'll use the + operator defined in the class SInt, rather than the (contextual) + operator defined by SIntRing.

And last update for the night (morning?): Seems like the only ops being overridden with new functionality are the ones found in Ring... (Mod just reduces to original Chisel %). I'm just going to add some more implicit conversions as a temp. solution...