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

Knowing when something is in a ring #75

Open
chick opened this issue Mar 2, 2017 · 20 comments
Open

Knowing when something is in a ring #75

chick opened this issue Mar 2, 2017 · 20 comments

Comments

@chick
Copy link
Contributor

chick commented Mar 2, 2017

The following circuit ignores the DspContext directive.

class BadUIntSubtractWithGrow extends Module {
  val io = IO(new Bundle {
    val a = Input(UInt(4.W))
    val b = Input(UInt(4.W))
    val o = Output(UInt(4.W))
  })
  val r = RegNext(DspContext.withOverflowType(Grow) { io.a - io.b })
  io.o := r
}

The reason is that since the IO's are not declared with generators that have been given a Ring constraint the - that is used is the basic chisel one and not the one in UInt Ring.
As a result this circuit compiles, despite that subtract with overflow type Grow is not supported

@chick
Copy link
Contributor Author

chick commented Mar 2, 2017

@shunshou @grebe I don't know if this is a big problem but it tripped me up writing a test for unsupported subtraction

@shunshou
Copy link
Member

shunshou commented Mar 2, 2017

Actually -- that's confusing. If you have an import implicits, would it still not implicitly say that UInt is Ring? If you didn't import implicits, then I think that's correct... (not sure if I really understand implicits though...)

@chick
Copy link
Contributor Author

chick commented Mar 2, 2017

I did use

import dsptools.numbers.implicits._

Maybe @grebe can comment on that

@shunshou
Copy link
Member

shunshou commented Mar 2, 2017

ok... I'm not sure if I like that behavior then :\ Definitely would've been a gotcha for me when writing control logic (i.e. control always UInt, but I still want some of the custom behaviors enabled with the typeclasses). But if that's the case, some big warning somewhere would be nice.

@chick
Copy link
Contributor Author

chick commented Mar 2, 2017

I don't like it either, I couldn't figure out at first why I wasn't getting exception I expected :P but I'm not sure what to do about it.

@shunshou
Copy link
Member

shunshou commented Mar 2, 2017

Could you, with the implicit stuff do some type conversion from UInt to UIntTypeClass?

@chick
Copy link
Contributor Author

chick commented Mar 9, 2017

@grebe @shunshou
I'm trying to test SignBit with the following

class SignBitTester(c: SignBit[UInt]) extends DspTester(c) {
  poke(c.io.uIn, 0)
  expect(c.io.uOut, 0.0)
}

class SignBit[TU <: Data : Ring](uGen: TU) extends Module {
  val io = IO(new Bundle {
    val uIn = Input(uGen)
    val uOut = Output(UInt(1.W))
  })

  io.uOut := io.uIn.signBit()
}

But the io.uIn.signBit() is a compile error. What should I do to type signature to pick this up. I've started to go through the permutations of UIntInteger, UIntImpl etc. Do we need to consider adding chisel3 level support for things like signBit

I can do

class SignBit(uGen: UInt, sGen: SInt, fGen: FixedPoint) extends Module {
  val io = IO(new Bundle {
    val uIn = Input(uGen)
    val sIn = Input(sGen)
    val fIn = Input(fGen)
    val uOut = Output(UInt(1.W))
    val sOut = Output(UInt(1.W))
    val fOut = Output(UInt(1.W))
  })

  io.uOut := io.uIn.signBit()
  io.sOut := io.sIn.signBit()
  io.fOut := io.fIn.signBit()
}

But this does not allow for generic number passing.

@chick
Copy link
Contributor Author

chick commented Mar 9, 2017

@shunshou also Numbers.md says the following, is that right

a.signBit
1 if a is zero or positive; 0 if a is negative

@shunshou
Copy link
Member

shunshou commented Mar 9, 2017

signBit is only on Data:RealBits, Data:IntegerBits. Ring doesn't have any notion of "bits". RealBits and IntegerBits should be all encompassing (of Ring, Order, etc.). Sorry! Numbers.md is a typo -- it should be 0 if a is zero or positive; 1 is a is negative. Good catch!

@chick
Copy link
Contributor Author

chick commented Mar 9, 2017

I'll change the spec with my next commit of more tests

@chick
Copy link
Contributor Author

chick commented Mar 9, 2017

So what is the right way to specify type parameters so one can apply signBit
to an instance of the a parameter that could be UInt SInt, FixedPoint or DspReal

@chick
Copy link
Contributor Author

chick commented Mar 9, 2017

@shunshou Numbers section on div2 what does
UInt: Consistent with a >> n (i.e. rounds 1/2 to 0)
mean. How can UInt be 1/2

@shunshou
Copy link
Member

shunshou commented Mar 9, 2017

class SignBit[TU <: Data : RealBits](uGen: TU) extends Module {
  val io = IO(new Bundle {
    val uIn = Input(uGen)
    val uOut = Output(UInt(1.W))
  })

  io.uOut := io.uIn.signBit()
}

Basically, Paul was using Data:Real to allow you access to anything more than +, -, * [Kind of confusing with DspReal] so I added some bit-related things by having RealBits extend Real.

As an example, Ring doesn't give you comparison -- so to be safe, if you wanted the whole range of Typeclass ops, you really should be using RealBits and not Ring. Only when you want to limit the supported operations to +, -, * should you use Ring. (Again, really confusing unless you understand type classes, which I don't...). >_>

@shunshou
Copy link
Member

shunshou commented Mar 9, 2017

I'm just saying that 1 >> 1 would return 0. Math-y 1 >> 1 should be 1 / 2^(1) == 1/2 (if you added more bit precision), but we don't do that.

Maybe my explanation was poorly written and just confusing to people instead of being helpful. Feel free to change it to something that makes sense to you.

@shunshou
Copy link
Member

@chick have you figured out how to give typeclass functionality to UInts? :\

like val test = UInt(3.W)

and then do some typeclass only thing on test? Should it just be done with some implicit conversion to UIntTypeClass? It's very confusing...

@shunshou
Copy link
Member

Actually, this is a huge problem that renders all of the typeclass stuff useless for all of my control logic.

@chick
Copy link
Contributor Author

chick commented Mar 13, 2017

I've fiddled with it a bit this weekend, I have not solved. I'll work on it more tonight and tomorrow and hopefully by the end of hacking tomorrow, we can some up with something

@chick
Copy link
Contributor Author

chick commented Mar 13, 2017

I agree this is a big problem.

@grebe
Copy link
Contributor

grebe commented Sep 7, 2017

I think this is resolved by our decision that operations should have their normal behavior (eg typeclass's + === UInt +) and the addition of context_+. I think this issue can be closed, although we should revisit how happy we are with the context_ approach.

@shunshou
Copy link
Member

shunshou commented Sep 7, 2017

definitely dislike having context_, but cleanly separating out somehow is important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants