Thorough DSP Tests

[shunshou]

Here's a bunch of scenarios I thought up that need to be tested. I hope i addressed these all in my code, but I could've missed some things... so it'd be nice if someone other than me tested. @chick

Sorry this might be confusing -- feel free to ask what I mean.

---

Test with Verilator + FirrtlInterpreter: mostly test w/ Verilator.

In general, don't need to test with too many bits, fractional bits (probably something like 8.W, 4.BP), but you should test across the full range of values (incrementing by say something like 0.05).

All DspContexts should be checked -- understand overflow behavior given Grow/Wrap and see that the result is correct. Good thing to have someone else who didn't write the code make the TB to ensure that the functionality is clear.

For SInt, Fixed, check that taking the absolute value of the most negative # supported by the width does the right thing -- it depends on Overflow behavior.

-(Most negative number) should also have different results based off of Context.

Inputs to operators should have different bitwidths and binary points to make sure things get padded out correctly (from brief experimentation, things look right).

Add/subtract/multiply numbers of different signs: ++, --, -+, +-
a + b might be like
q = Seq( -3.7, -3.3, -2.7, -2.2, -1.7, -1.1, -0.7, -0.5, -0.4, 0.0, 0.4, 0.7, 1.1, 1.7, 2.2,2.7, 3.3, 3.7)
a = q ++ q.reverse
b = q ++ q
Make sure that rounding in either direction is tested (i.e. -3.7, 3.3 have different outputs)

When testing shift left, shift right, make sure you also shift more than the bitwidth (shift in increments of 1) to test out expected behavior.

Test connection from DspReal/FixedPoint to FixedPoint/DspReal for golden model verification (even though the design isn't synthesizable).

Check that ops requiring using different rounding modes: FixedPoint multiply, FixedPoint div2, Round, etc. (refer to README) overflow correctly (context dependent) when rounding up (and the original value is the max representable).

Check that Chisel + Firrtl known widths and known binary points match up. For example, setBinaryPoint doesn't seem to work when more binary points than current are specified (only in Chisel). In Chisel, the # of fractional bits increase, but the width stays the same, implying you're losing MSBs. Firrtl does this properly, but if I'm query-ing the Chisel KnownWidth, I'd be mislead. In general, the Chisel + Firrtl width + binary point inference for anything known should be double checked.

Try changing DspContext @ different levels: Module, operation, top level design and make sure the context is correct within the {}. Also, I assume the user might have

class XMod extends Module {
  DspContext.blah.withValue(x) {
    /* code */ 
  }
}

vs.

class TopMod extends Module {
...
  DspContext.blah.withValue(y) {
    val inst = new XMod()
  }
...
}

Double check that add/multiply delays work -- especially for complex multiply + comples.abssq. Check that in both those cases, trim + overflow work too. (More interesting use case since they require both add and multiply -- so lots of context options come into play). Try for different delays and make sure that DspContext.complexMulDly changes when numAddPipes and numMulPipes changes.

Check that Width < BinaryPoint is legal and correct.

---

When you do ops on FixedPoint numbers, your scala.math operations should mimic the results exactly (bit accurate) based off of the binaryPoint. i.e. FixedTolLSBs should be 0, and expects should pass.

Test bit growth of 1 (especially common) for things like FixedPoint multiply, and make sure the result is bit accurate. You should be able to expect how the trimming works.

---

Also, purposely come up with a case where having tolerance = 0 fails but tolerance of say 1-2 LSBs works to test out that Tester tolerances work.

For the sin, cos, tan approximations in Verilator, I tested with expect tolerance of 1e-8, and for most reasonable inputs, they seemed to work, but I'm not sure if I was 100% comprehensive.

[chick]

@shunshou Thanks I'll work my way through these

[chick]

@shunshou seems to me that UInt ring stuff should error on subtraction. Firrtl Spec says that UInt - UInt => SInt but Ring says minus returns UInt. So following, where io.a and io.b are UInts

  val regSubWrap = RegNext(DspContext.withOverflowType(Wrap) { io.a - io.b })

gives

firrtl.passes.CheckTypes$InvalidConnect:  @[Reg.scala 14:44:@23.4]: [module OverflowTypeCircuit]  Type mismatch. Cannot connect regSubGrow to _T_16.

Seems like a better error up front, saying to cast to SInt would be more reasonable

[shunshou]

This is maybe where @grebe states his opinion. Inside the Ring (so the user doesn't have to do it), we can do an explicit cast back to UInt. I think the types should frankly stay consistent w/ their input types (no surprises!).

[chick]

@shunshou @grebe Something like this then?

  override def minus(f: UInt, g: UInt): UInt = {
    val diff = context.overflowType match {
      case Grow => f.asSInt -& g.asSInt
      case Wrap => f.asSInt -% g.asSInt
      case _ => throw DspException("Saturating subtractor hasn't been implemented")
    }
    ShiftRegister(diff.asUInt, context.numAddPipes)
  }

[shunshou]

I'm fine with that. @grebe?

[chick]

@shunshou @grebe So with above logic subtracting 4-bit UInts from each other gives

15 - 1 => 30

Is that ok?

[shunshou]

Oh for wrap it should be trimmed back to 4 bits. I thought that that was the whole point of -% ?

[shunshou]

Is there no -% on UInt that returns a 4 bit UInt (or SInt that should then be cast as UInt)? b/c if so, IMO, that functionality isn't "Wrap"

[shunshou]

Also, re -&, what should be the behavior? That's one of those weird cases where, to always be mathematically correct, you should convert to an SInt... Or in that case, should you only make guarantees on outputs when they're >=0? @grebe ?

[grebe]

Sorry, I fell very behind on my github mentions. I think we agreed to throw an exception for -&

[chick]

@shunshou So what is
io.sAbsWrap := DspContext.withOverflowType(Wrap) { io.sIn.abs() }
Supposed to do for an SInt(4.W) with value -8

[shunshou]

I believe it'll be -8, right? Wrap has 0 guarantees on mathematical correctness, and is just intended to do whatever the standard bit operation does. You'd have a separate function that might pull it to 7 or something. Or maybe Wrap is a bad descriptor? @grebe, opinions?

[chick]

Yes, -8 is what it returns. So I guess it's working right Chick Markley Staff Programmer -- Aspire Lab University of California, Berkeley 583-2 Soda Hall, Berkeley, CA 94720-1776 [email protected]

…

On Thu, Mar 2, 2017 at 4:45 PM, Angie Wang ***@***.***> wrote: I believe it'll be -8, right? Wrap has 0 guarantees on mathematical correctness, and is just intended to do whatever the standard bit operation does. You'd have a separate function that might pull it to 7 or something. Or maybe Wrap is a bad descriptor? @grebe <https://github.com/grebe>, opinions? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#70 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA4WVxbtVNYWcZXO6GbvHOcGphrH-IkPks5rh2LGgaJpZM4MDm_P> .

[shunshou]

Reiterating what I said in the new issue -- test that a sequence of typeclass ops handles dspcontext correctly (i.e. does some op, b/c of the way it's implemented, make you lose the Real/Ring, etc. context).