Mux1H Bug (HELP!) OneHotMux broken when used with FixedPoint #558

shunshou · 2017-03-24T20:06:54Z

Please fix ASAP! I reallyreallyreally would like this to work.

package dspblocks.fft
import chisel3._
import chisel3.experimental._
import dsptools.numbers._
import dsptools.numbers.implicits._
import org.scalatest.{FlatSpec, Matchers}
import dsptools.{DspTesterOptionsManager, DspTester}

class Borked[T <: Data:RealBits](dspDataType: => T) extends Module {
  val io = IO(new Bundle {
    val sels = Input(Vec(4, Bool()))
    val out = Output(DspComplex(dspDataType))
  })

  val one = dspDataType.fromDoubleWithFixedWidth(1.0)
  val C52 = dspDataType.fromDoubleWithFixedWidth(-1.5388417840003967)
  val C75 = dspDataType.fromDoubleWithFixedWidth(-0.34087294340133667)
  val A5 = Mux1H(Seq(
    io.sels(0) -> one,
    io.sels(1) -> C52,
    io.sels(2) -> C75,
    io.sels(3) -> one
  ))
  io.out.real := A5
}

class BorkedSpec extends FlatSpec with Matchers {
  behavior of "Borked"
  it should "not suck" in {
    val opt = new DspTesterOptionsManager 
    dsptools.Driver.execute(() => new Borked(dspDataType = FixedPoint(28.W, 24.BP)), opt) { c =>
      new BorkedTester(c)
    } should be (true)
  }
}

class BorkedTester[T <: Data:RealBits](c: Borked[T]) extends DspTester(c) {
  poke(c.io.sels(0), false.B)
  poke(c.io.sels(1), false.B)
  poke(c.io.sels(3), false.B)
  poke(c.io.sels(2), true.B)
  peek(c.io.out.real)
}

I spent a really long time figuring out why my circuit was working with DspReal but not with FixedPoint (and running into annoying issues like Mux not supporting different BP's, etc.), and I just can't afford to be spending this time debugging stuff on the Chisel side.

This works:

package dspblocks.fft
import chisel3._
import chisel3.experimental._
import dsptools.numbers._
import dsptools.numbers.implicits._
import org.scalatest.{FlatSpec, Matchers}
import dsptools.{DspTesterOptionsManager, DspTester}

class Borked[T <: Data:RealBits](dspDataType: => T) extends Module {
  val io = IO(new Bundle {
    val sels = Input(Vec(4, Bool()))
    val out = Output(DspComplex(dspDataType))
  })

  val one = Wire(dspDataType)
  one := dspDataType.fromDoubleWithFixedWidth(1.0)
  val C52 = Wire(dspDataType)
  C52 := dspDataType.fromDoubleWithFixedWidth(-1.5388417840003967)
  val C75 = Wire(dspDataType)
  C75 := dspDataType.fromDoubleWithFixedWidth(-0.34087294340133667)
  val A5 = Mux1H(Seq(
    io.sels(0) -> one,
    io.sels(1) -> C52,
    io.sels(2) -> C75,
    io.sels(3) -> one
  ))
  io.out.real := A5
}

class BorkedSpec extends FlatSpec with Matchers {
  behavior of "Borked"
  it should "not suck" in {
    val opt = new DspTesterOptionsManager 
    dsptools.Driver.execute(() => new Borked(dspDataType = FixedPoint(28.W, 24.BP)), opt) { c =>
      new BorkedTester(c)
    } should be (true)
  }
}

class BorkedTester[T <: Data:RealBits](c: Borked[T]) extends DspTester(c) {
  poke(c.io.sels(0), false.B)
  poke(c.io.sels(1), false.B)
  poke(c.io.sels(3), false.B)
  poke(c.io.sels(2), true.B)
  peek(c.io.out.real)
}

Only difference is I assign the constant to a Wire before feeding into the Mux1H (who would've thought that was necessary?)

The previous thing sometimes works (don't know what's the right/wrong condition).

Note that I've separately checked the internal C## constant values to make sure they're right. -- Separately @chick The day before yesterday, I ran into a serious Verilator-did-the-wrong-thing bug (which I have no idea how to get fixed..., since who do you even contact about Verilator bugs???), so I sanity checked against Firrtl-interpreter to make sure this was a Chisel bug. Firrtl-interpreter didn't seem to let me peek non-port signals. I could printf, but printf returned the LongBits version instead of some easy to understand Double. Punching signals out was a huge pain.

The text was updated successfully, but these errors were encountered:

aswaterman · 2017-03-24T20:10:43Z

Could you provide the Firrtl for the two cases?

shunshou · 2017-03-24T20:14:13Z

Correct case:

circuit Borked : 
  module Borked : 
    input clock : Clock
    input reset : UInt<1>
    output io : {flip sels : UInt<1>[4], out : {real : Fixed<28><<24>>, imag : Fixed<28><<24>>}}
    
    io is invalid
    io is invalid
    wire one : Fixed<28><<24>> @[Chisel_Broken_Debug.scala 15:17]
    one is invalid @[Chisel_Broken_Debug.scala 15:17]
    one <= asFixedPoint(UInt<26>("h01000000"), 24) @[Chisel_Broken_Debug.scala 16:7]
    wire C52 : Fixed<28><<24>> @[Chisel_Broken_Debug.scala 17:17]
    C52 is invalid @[Chisel_Broken_Debug.scala 17:17]
    C52 <= asFixedPoint(UInt<26>("h02760e77"), 24) @[Chisel_Broken_Debug.scala 18:7]
    wire C75 : Fixed<28><<24>> @[Chisel_Broken_Debug.scala 19:17]
    C75 is invalid @[Chisel_Broken_Debug.scala 19:17]
    C75 <= asFixedPoint(UInt<24>("h0a8bc8d"), 24) @[Chisel_Broken_Debug.scala 20:7]
    node T_28 = asUInt(one) @[Mux.scala 19:72]
    node T_30 = mux(io.sels[0], T_28, UInt<1>("h00")) @[Mux.scala 19:72]
    node T_31 = asUInt(C52) @[Mux.scala 19:72]
    node T_33 = mux(io.sels[1], T_31, UInt<1>("h00")) @[Mux.scala 19:72]
    node T_34 = asUInt(C75) @[Mux.scala 19:72]
    node T_36 = mux(io.sels[2], T_34, UInt<1>("h00")) @[Mux.scala 19:72]
    node T_37 = asUInt(one) @[Mux.scala 19:72]
    node T_39 = mux(io.sels[3], T_37, UInt<1>("h00")) @[Mux.scala 19:72]
    node T_41 = or(T_30, T_33) @[Mux.scala 19:72]
    node T_42 = or(T_41, T_36) @[Mux.scala 19:72]
    node T_43 = or(T_42, T_39) @[Mux.scala 19:72]
    wire A5 : Fixed<28><<24>> @[Mux.scala 19:72]
    A5 is invalid @[Mux.scala 19:72]
    node T_45 = asFixedPoint(T_43, 24) @[Mux.scala 19:72]
    A5 <= T_45 @[Mux.scala 19:72]
    io.out.real <= A5 @[Chisel_Broken_Debug.scala 27:15]

Incorrect case:

circuit Borked : 
  module Borked : 
    input clock : Clock
    input reset : UInt<1>
    output io : {flip sels : UInt<1>[4], out : {real : Fixed<28><<24>>, imag : Fixed<28><<24>>}}
    
    io is invalid
    io is invalid
    node T_22 = asUInt(asFixedPoint(UInt<26>("h01000000"), 24)) @[Mux.scala 19:72]
    node T_24 = mux(io.sels[0], T_22, UInt<1>("h00")) @[Mux.scala 19:72]
    node T_25 = asUInt(asFixedPoint(UInt<26>("h02760e77"), 24)) @[Mux.scala 19:72]
    node T_27 = mux(io.sels[1], T_25, UInt<1>("h00")) @[Mux.scala 19:72]
    node T_28 = asUInt(asFixedPoint(UInt<24>("h0a8bc8d"), 24)) @[Mux.scala 19:72]
    node T_30 = mux(io.sels[2], T_28, UInt<1>("h00")) @[Mux.scala 19:72]
    node T_31 = asUInt(asFixedPoint(UInt<26>("h01000000"), 24)) @[Mux.scala 19:72]
    node T_33 = mux(io.sels[3], T_31, UInt<1>("h00")) @[Mux.scala 19:72]
    node T_35 = or(T_24, T_27) @[Mux.scala 19:72]
    node T_36 = or(T_35, T_30) @[Mux.scala 19:72]
    node T_37 = or(T_36, T_33) @[Mux.scala 19:72]
    wire A5 : Fixed<26><<24>> @[Mux.scala 19:72]
    A5 is invalid @[Mux.scala 19:72]
    node T_39 = asFixedPoint(T_37, 24) @[Mux.scala 19:72]
    A5 <= T_39 @[Mux.scala 19:72]
    io.out.real <= A5 @[Chisel_Broken_Debug.scala 24:15]

shunshou · 2017-03-24T20:22:06Z

I guess, for the Mux, a strict asUInt on the inputs isn't sufficient and you need to width/sign match first... Or rather -- why does this use UInt instead of SInt?

aswaterman · 2017-03-24T20:24:27Z

I'm concerned about the type of A5 differing in the two cases.

shunshou · 2017-03-24T20:27:36Z

Oh crap. I didn't even notice that. Fundamentally, in one case, I'm forcing the input widths to all be the same and in the other case, I'm not, although that's theoretically not necessary, since the tools should know to auto pad properly...

shunshou · 2017-03-24T20:30:59Z

One more thing: The wrong output was:

PEEK Borked.io_out_real -> 0.6591270565986633, Q3.24

Hard to see in bits, but it was definitely positive when it should've been negative...

aswaterman · 2017-03-24T20:49:13Z

I believe something like this might fix it: https://gist.github.com/aswaterman/5c0aee3ef33179c4e821eaafc27c7e1d

But this isn't quite complete, because it creates a crap ton of extra wires. @ducky64 @jackkoenig you might consider incorporating a patch like this, which only adds the wires for type conversions for inputs whose type differs from the output type.

chick · 2017-03-24T21:23:11Z

@shunshou This seems to fixed the BorkedSpec tests. I'll push it and let @ducky64 and/or @jackkoenig review

chick · 2017-03-24T21:36:42Z

@shunshou Andrew's fix is PR'd as #560

shunshou · 2017-03-24T21:37:21Z

Thx I'll give it a shot in a bit.

…

On Fri, Mar 24, 2017 at 2:36 PM, Chick Markley ***@***.***> wrote: @shunshou <https://github.com/shunshou> Andrew's fix is PR'd as #560 <#560> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#558 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGTTFoza-_qfH62VFlGwwwV4qI0U7sArks5rpDdrgaJpZM4Motpd> .

chick · 2017-03-24T21:37:57Z

@shunshou I'm going to be off-line for an hour. If this get approval right away, please merge, or ask @ucbjrl to do it.

chick · 2017-03-28T05:26:50Z

@shunshou I am convinced at this point that this is a firrtl error. I'm going to look a little more into it now, but I'm pretty sure that chisel is not to blame. I'm going to construct an example. Hopefully @azidar will see this soon

ducky64 · 2017-04-18T19:38:27Z

Appears fixed by #573.
Reopen if it's still broken

shunshou added the bug label Mar 24, 2017

shunshou assigned shunshou, ducky64 and chick and unassigned shunshou Mar 24, 2017

shunshou mentioned this issue Mar 24, 2017

Does fromDoubleWithFixedWidth work? ucb-bar/dsptools#90

Open

chick mentioned this issue Mar 24, 2017

Andrew's fix for one-hot mux with differing fixed point targets #560

Closed

chick changed the title ~~Mux1H Bug (HELP!)~~ Mux1H Bug (HELP!) OneHotMux broken when used with FixedPoint Mar 31, 2017

chick mentioned this issue Apr 3, 2017

Fix one hot mux #573

Merged

ducky64 closed this as completed Apr 18, 2017

ucbjrl unassigned chick Jun 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mux1H Bug (HELP!) OneHotMux broken when used with FixedPoint #558

Mux1H Bug (HELP!) OneHotMux broken when used with FixedPoint #558

shunshou commented Mar 24, 2017

aswaterman commented Mar 24, 2017

shunshou commented Mar 24, 2017 •

edited

Loading

shunshou commented Mar 24, 2017 •

edited

Loading

aswaterman commented Mar 24, 2017

shunshou commented Mar 24, 2017

shunshou commented Mar 24, 2017 •

edited

Loading

aswaterman commented Mar 24, 2017

chick commented Mar 24, 2017

chick commented Mar 24, 2017

shunshou commented Mar 24, 2017 via email

chick commented Mar 24, 2017

chick commented Mar 28, 2017

ducky64 commented Apr 18, 2017

Mux1H Bug (HELP!) OneHotMux broken when used with FixedPoint #558

Mux1H Bug (HELP!) OneHotMux broken when used with FixedPoint #558

Comments

shunshou commented Mar 24, 2017

aswaterman commented Mar 24, 2017

shunshou commented Mar 24, 2017 • edited Loading

shunshou commented Mar 24, 2017 • edited Loading

aswaterman commented Mar 24, 2017

shunshou commented Mar 24, 2017

shunshou commented Mar 24, 2017 • edited Loading

aswaterman commented Mar 24, 2017

chick commented Mar 24, 2017

chick commented Mar 24, 2017

shunshou commented Mar 24, 2017 via email

chick commented Mar 24, 2017

chick commented Mar 28, 2017

ducky64 commented Apr 18, 2017

shunshou commented Mar 24, 2017 •

edited

Loading

shunshou commented Mar 24, 2017 •

edited

Loading

shunshou commented Mar 24, 2017 •

edited

Loading