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

Did you guys actually fix fixed point tester problems? #47

Closed
shunshou opened this issue Jan 17, 2017 · 7 comments
Closed

Did you guys actually fix fixed point tester problems? #47

shunshou opened this issue Jan 17, 2017 · 7 comments

Comments

@shunshou
Copy link
Member

I'm not really sure how any fixed point hardware could've possibly tested out correctly.

I think this tester behavior is the same as in Chisel2:

If you trace a poke, in the backend, the BigInt to the DUT is signed, whereas, if you trace a peek, from the backend, the BigInt is unsigned (although, depending on the data type, it should be interpretted as signed).

This is problematic because:

  1. For chisel-testers, if you want to peek/poke an SInt and then do a compare, the poked (BigInt) input won't equal the unsigned peeked output. Sure, you could convert the value to signed and then things check out.

  2. For dsptesters, the dspExpects should fail on Fixed for the same reason. The doubles that are compared don't make sense for signed outputs. i.e.

expect got 65534.50000000 expect -1.50000000 ( w/ FixedPoint(32.W, 16.BP)).


I recall vaguely that there was some talk about sign conversion, but it doesn't look like this was ever done properly?

The old Chisel2 tester had a sign conversion function that worked...

Am I missing something???

@grebe @chick @stevobailey

@shunshou
Copy link
Member Author

Point is, I can go around and fix this at a lower level, but that fundamentally might change the way you guys do your tests if you do correct for this somewhere that I'm not aware of...

@shunshou
Copy link
Member Author

Also, nearly equals is insufficient for fixed point expects. I'm working on a better solution, which is essentially porting over my old Chisel2 DSPTester to here.

@stevobailey
Copy link
Contributor

Pull request #37 adds "toBigIntUnsigned" and "toDoubleFromUnsigned". Perhaps not the best names, but they convert between FixedPoint and unsigned integers (or rather, unsigned representations of the bits in a signed FixedPoint number). For our stuff, we have AXI everywhere, which abstracts all input/output types as UInts. The way I've written it works for FixedPoint up to 32.W I believe, but not any larger yet.

Also, in the same file (DspBlock.scala, which got moved to rocket-dsp-utils) we have a compareOutput def that compares two arrays. The intention is to just stream data in and out and wait to compare at the end. compareOutput lets you supply an "epsilon" as a "nearly equals". Future work will do more interesting comparisons, like noise analysis or more holistic "expect"s.

@shunshou
Copy link
Member Author

My old tester basically allowed you to set tolerances for fixed point and real expects. Even for reals, you're going to be off by a fudge factor that grows as the computations complexity increases. For fixed point expects, I had a tolerance in terms of "how many LSBs" can be wrong. Noise analysis (done @ the block level, and not part of ChiselDSP) just took the Double out and compared to the real case (for SQNR).

I'm adding my expect functionality back into VerboseDspTester.


Re the unsigned stuff:

It would've made more sense to make all of these changes directly in DspTester, since not everyone will be starting out with the AXI interface.

There are 2 things I can do:

  1. Overhaul DspTester to handle signed stuff properly. I assume this affects your DspBlockTester.
  2. Make all of my changes to VerboseDspTester. You can choose to have your DspBlockTester extend it or not.

By overhaul, I mean move sign flipping to the appropriate tester. I should also be able to get it to work for Fixed > 32, since what they did in Chisel2 should work just fine.

What's your preference? @stevobailey @grebe

@shunshou
Copy link
Member Author

Oh, tho frankly speaking, this might be an easier change @ the chisel-testers level... I'm just concerned that a PR will take ages to resolve there... [granted, I've already forked it...]. @chick do you have any opinions?

Basically, I need patches for these things in ~day timeframes...

@grebe
Copy link
Contributor

grebe commented Jan 17, 2017

@shunshou I'd rather see stuff wind up in the DspTester where possible rather than a VerboseDspTester. @chick would be better able to tell you about chisel-testers, although I don't think it would take much longer to get stuff looked at.

@shunshou
Copy link
Member Author

Ok well VerboseDspTester makes some amount of sense for my additional VerilogDumping features, and DspTester is kind of a hot mess right now [functionality is possibly correct, but things are a little all over the place...].

If you have some way to do a regression test, I can make changes in DspTester as long as your stuff doesn't break... I just posted an issue in chisel-testers.

@shunshou shunshou closed this as completed Feb 7, 2017
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