treadle icon indicating copy to clipboard operation
treadle copied to clipboard

Published 20 hours ago • verified publisher icon chipsalliance

Incorrect BlackBoxes input values changes

Open johnsbrew opened this issue 5 years ago • 9 comments

Hi,

I am running into an issue with a simple fallthrough fifo black-boxing. I have a complex module with many fifos which pass successfully its test with verilator. Test crashes with treadle backend and demonstrates an unexpected behavior: in the blackbox scala implementation, the data input din changes just before clock posedge...

Here is the partial scala context for reference

  val prio_req      = Reg(new Req)
  val prio_valid    = RegInit(Bool(), false.B)
  val prio_ready    = Wire(Bool())

  when(prio_ready){
      prio_valid    := mf.io.res.valid
      prio_req      := mf.io.res.bits.extra
  }

  val sip_ready  = Wire(Bool())  
  val sip_valid  = RegInit(Bool(), false.B)

  val sip_req    = Reg(new ReqHashed)
  // BlackBoxed Module
  val sip_ff = Module(new FallthroughFifo(
    WIDTH       = prio_req.getWidth,
    MAX_DEPTH   = 32,
    LOW_LATENCY = false
  ))
// Both signal are only updated at posedge clock 
  sip_ff.io.din   := prio_req.asUInt
  sip_ff.io.wr_en := prio_valid && prio_ready

Here is what I get on the waves (just as expected)

Capture d’écran 2019-08-01 à 15 37 29

Here is what I get when I add some debug print in the black box implementation (not what I expect, and thus inconsistent with the waves)

-------- @PosEdge -------
change on [wr_en] : 0
change on [din] : 0
change on [wr_en] : 0
change on [din] : 0
change on [wr_en] : 0
change on [din] : 0
change on [wr_en] : 0
change on [din] : 0
change on [wr_en] : 0
change on [din] : 0
change on [wr_en] : 0
change on [din] : 0
change on [wr_en] : 0
change on [din] : 598400000002862e9 <<<<<<< This should not happen before posedge
 -------- @PosEdge -------
change on [wr_en] : 1
change on [din] : 598400000002862e9
change on [wr_en] : 1
change on [din] : 598400000002862e9
change on [wr_en] : 1
change on [din] : 598400000002862e9
change on [wr_en] : 1
change on [din] : 598400000002862e9
change on [wr_en] : 1
change on [din] : 598400000002862e9
change on [wr_en] : 1
change on [din] : 598400000002862e9
 -------- @PosEdge -------
   Queue: 598400000002862e9
change on [wr_en] : 0
change on [din] : 598400000002862e9
change on [wr_en] : 0
change on [din] : 598400000002862e9
change on [wr_en] : 0
change on [din] : 598400000002862e9
change on [wr_en] : 0
change on [din] : 598400000002862e9
change on [wr_en] : 0
change on [din] : 598400000002862e9
change on [wr_en] : 0
change on [din] : 598400000002862e9
 -------- @PosEdge -------
change on [wr_en] : 0
change on [din] : 598400000002862e9
change on [wr_en] : 0
change on [din] : 598400000002862e9
change on [wr_en] : 0
change on [din] : 598400000002862e9
change on [wr_en] : 0
change on [din] : 598400000002862e9
change on [wr_en] : 0
change on [din] : 4924000000015fb14 <<<<<<< This should not happen here
 -------- @PosEdge -------
change on [wr_en] : 1
change on [din] : 4924000000015fb14
change on [wr_en] : 1
change on [din] : 4924000000015fb14
change on [wr_en] : 1
change on [din] : 4924000000015fb14
change on [wr_en] : 1
change on [din] : 4924000000015fb14
change on [wr_en] : 1
change on [din] : 4924000000015fb14
change on [wr_en] : 1
change on [din] : 4924000000015fb14
 -------- @PosEdge -------
   Queue: 4924000000015fb14
change on [wr_en] : 0
change on [din] : 4924000000015fb14
change on [wr_en] : 0
change on [din] : 4924000000015fb14
change on [wr_en] : 0
change on [din] : 4924000000015fb14
change on [wr_en] : 0
change on [din] : 4924000000015fb14
change on [wr_en] : 0
change on [din] : 4924000000015fb14
change on [wr_en] : 0
change on [din] : 4924000000015fb14
change on [wr_en] : 0
change on [din] : 6f84000000009d82c <<<<<<< This should not happen here
 -------- @PosEdge -------
change on [wr_en] : 1
change on [din] : 6f84000000009d82c
change on [wr_en] : 1
change on [din] : 6f84000000009d82c
change on [wr_en] : 1
change on [din] : 6f84000000009d82c
change on [wr_en] : 1
change on [din] : 6f84000000009d82c
change on [wr_en] : 1
change on [din] : 6f84000000009d82c
change on [wr_en] : 1
change on [din] : 4ee400000001892e0 <<<<<<< This should not happen here
 -------- @PosEdge -------
   Queue: 4ee400000001892e0 <<<<<<< Wrong data being validated
change on [wr_en] : 1
change on [din] : 4ee400000001892e0
change on [wr_en] : 1
change on [din] : 4ee400000001892e0
change on [wr_en] : 1
change on [din] : 4ee400000001892e0
change on [wr_en] : 1
change on [din] : 4ee400000001892e0
change on [wr_en] : 1
change on [din] : 4ee400000001892e0

/* issue propagates ... */

Personal thoughts:

  • I don't understand why wr_en & din are changing multiple times within a single step ?
  • I don't understand why wr_en & dindo not behave in the same way ? Could it be due to the cast of my custom Bundle Req as UInt ?

@chick , any idea ?

johnsbrew avatar Aug 01 '19 13:08 johnsbrew

Taking a look at this. I can't really tell what's going on with our situation. I have almost completed a Fall through FIFO of my own, but it's a bit complicated. I hope to finish it within the hour or failing that in the morning. I'll share it when I get it going. I don't see anything obviously wrong with what's treadle is doing yet, but there could certainly be something. If you could share (in a gist) or otherwise the scala black box, I can compare it against my attempt tomorrow. I hope to get this fixed quickly

chick avatar Aug 02 '19 05:08 chick

Hi @chick, thank you for your interest in this issue. Here is the scala blackbox file : https://gist.github.com/johnsbrew/c4b0ce91b30a7b3097589331eccd258d I have also attached the test file for the module. Unfortunately this test does not trigger the bug described above (as far as I have seen). I get results as shown below that are precisely what I would have expected

-------- @PosEdge -------
   Queue: b5
change on [din] : b5
change on [wr_en] : 1
change on [din] : 1d7
change on [wr_en] : 1
change on [din] : 1d7
change on [wr_en] : 1
 -------- @PosEdge -------
   Queue: 1d7
ff_inst Dequeue: 2ca
change on [din] : 1d7
change on [wr_en] : 1
change on [din] : 196
change on [wr_en] : 0
change on [din] : 196
change on [wr_en] : 0
 -------- @PosEdge -------
change on [din] : 196
change on [wr_en] : 0
change on [din] : 182
change on [wr_en] : 0
change on [din] : 182
change on [wr_en] : 0
 -------- @PosEdge -------
ff_inst Dequeue: 117
change on [din] : 182
change on [wr_en] : 0
change on [din] : 29d
change on [wr_en] : 0
change on [din] : 29d
change on [wr_en] : 0
 -------- @PosEdge -------
change on [din] : 29d
change on [wr_en] : 0
change on [din] : 77
change on [wr_en] : 1
change on [din] : 77
change on [wr_en] : 1
 -------- @PosEdge -------
   Queue: 77
change on [din] : 77
change on [wr_en] : 1
change on [din] : 2b6
change on [wr_en] : 0
change on [din] : 2b6
change on [wr_en] : 0

I suspect that the issue comes from treadle struggling with the interpretation of my big host module.

johnsbrew avatar Aug 02 '19 08:08 johnsbrew

I just added you as a collaborator on a private repo of mine where I tried to get a version of what you provide to run. It currently runs and passes for me, but I really don't know if this is meaningful. Please take a look and let me know what you think I added some notes in the README.md Hopefully it is at least a step in the right direction.

Chick Markley Staff Programmer -- Adept Lab University of California, Berkeley 583-2 Soda Hall, Berkeley, CA 94720-1776 [email protected]

On Fri, Aug 2, 2019 at 1:23 AM johnsbrew [email protected] wrote:

Hi @chick https://github.com/chick, thank you for your interest in this issue. Here is the scala blackbox file : https://gist.github.com/johnsbrew/c4b0ce91b30a7b3097589331eccd258d I have also attached the test file for the module. Unfortunately this test does not trigger the bug described above (as far as I have seen). I get results as shown below that are precisely what I would have expected

-------- @PosEdge ------- Queue: b5 change on [din] : b5 change on [wr_en] : 1 change on [din] : 1d7 change on [wr_en] : 1 change on [din] : 1d7 change on [wr_en] : 1 -------- @PosEdge ------- Queue: 1d7 ff_inst Dequeue: 2ca change on [din] : 1d7 change on [wr_en] : 1 change on [din] : 196 change on [wr_en] : 0 change on [din] : 196 change on [wr_en] : 0 -------- @PosEdge ------- change on [din] : 196 change on [wr_en] : 0 change on [din] : 182 change on [wr_en] : 0 change on [din] : 182 change on [wr_en] : 0 -------- @PosEdge ------- ff_inst Dequeue: 117 change on [din] : 182 change on [wr_en] : 0 change on [din] : 29d change on [wr_en] : 0 change on [din] : 29d change on [wr_en] : 0 -------- @PosEdge ------- change on [din] : 29d change on [wr_en] : 0 change on [din] : 77 change on [wr_en] : 1 change on [din] : 77 change on [wr_en] : 1 -------- @PosEdge ------- Queue: 77 change on [din] : 77 change on [wr_en] : 1 change on [din] : 2b6 change on [wr_en] : 0 change on [din] : 2b6 change on [wr_en] : 0

I suspect that the issue comes from treadle struggling with the interpretation of my big host module.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/freechipsproject/treadle/issues/116?email_source=notifications&email_token=AAHBMV6WXB4MQPP4PBT2HJ3QCPVIRA5CNFSM4IIQ4PT2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3NBKBQ#issuecomment-517608710, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHBMV3XJP4PF4EDVE3RWSTQCPVIRANCNFSM4IIQ4PTQ .

chick avatar Aug 02 '19 23:08 chick

Hi @chick, sorry for the delay and thank you very much for taking some time to dig into this issue. I am not looking further down into the blackbox implementation as I cannot reproduce any unexpected behavior right into it. I am much more concerned about the precise order through which the events get executed. I enabled verbose mode of treadle to get some insight and here is what I got :

inst.sip_ff.clock <=1 h1
inst.sip_ff.io_din <=103204489060824933097 h598400000002862e9
change on [din] : 598400000002862e9
inst.sip_ff.ff_inst.clk <=1 h1
 -------- @PosEdge -------
inst.sip_ff.ff_inst : clock inst.sip_ff.ff_inst.clk state (PositiveEdge)

This ring a bell (clock propagation through ff_inst) about the way I wrapped my blackbox (in a Module and not ExtModule as you.

Please find my version below, do you see anything obviously wrong ?

import chisel3._
import chisel3.util._
import chisel3.experimental._
import collection.mutable.{Queue}


class FallthroughFifoBlackBox[T <: Record](params: Map[String, Param], ios: T) extends BlackBox(params) with HasBlackBoxResource {
  val io = IO(ios)
  // required to match actual verilog module name
  override def desiredName = "fallthrough_fifo"
  // main verilog file
  addResource("/fallthrough_fifo.sv")
  // dependencies
  addResource("/small_fifo.sv")

}

class FallthroughFifo(
  val WIDTH              : Int,
  val MAX_DEPTH          : Int,
  val LOW_LATENCY        : Boolean = false,
  val NEARLY_FULL_THRESH : Option[Int] = None, 
  val FORCE_ULTRARAM     : Boolean = false
) extends Module {
  // Constructor: process parameters
  var params : Map[String, Param] = Map(
    "WIDTH" -> WIDTH,
    "LOW_LATENCY" -> LOW_LATENCY,
    "MAX_DEPTH" -> MAX_DEPTH,
    "USE_AVAILABLE_SLOT" -> true,
    "FORCE_ULTRARAM" -> FORCE_ULTRARAM
  )

  NEARLY_FULL_THRESH match {
    case Some(i) => params += ("NEARLY_FULL_THRESH" -> i)
    case None    => // default : hardware behaviour
  }
  /* FallthroughFifoIO
   * Defines IOs to be shared between wrapper & black-box
   * Intra class definition to avoid to pass params
   */
  class FallthroughFifoIO extends Bundle {
    val wr_en = Input(Bool())
    val din   = Input(UInt(WIDTH.W))
  
    val rd_en = Input(Bool())
    val dout  = Output(UInt(WIDTH.W))
  
    val full = Output(Bool())
    val nearly_full = Output(Bool())
    val empty = Output(Bool())
    val available_slots = Output(UInt(log2Up(MAX_DEPTH).W+1.W))
  }
  val io = IO(new FallthroughFifoIO())
  // > Additional Black-box IOs (clock & reset)
  class FallthroughFifoBlackBoxIO extends FallthroughFifoIO {
    val clk = Input(Clock()) 
  }
  // Instanciate blackbox
  val ff_inst = Module(new FallthroughFifoBlackBox(params, new FallthroughFifoBlackBoxIO()))
  // Connections: 
  // > special case for clock (NB: and reset if any)
  ff_inst.io.clk <> clock
  // > automated for all forwarded IOs 
  for((i_name, i) <- io.elements){
    ff_inst.io.elements(i_name) <> i
  }
}

johnsbrew avatar Aug 06 '19 17:08 johnsbrew

I don't see anything obviously wrong. Treadle's order is done off of a topological sort of the symbol dependencies. There could be some problems there. The scala black boxes have not been used a whole lot (let alone on complicated/interesting designs). There could be some problems in there. I am planning on adding a capability of adding VCD events from internal events within the scala black box. That might provide better clues to possible implementation errors. If you think that might help I can up the priority there. Cheers

chick avatar Aug 07 '19 19:08 chick

Hi @chick, I spent some time trying to investigate myself the topological sorting within treadle. Multiples times I thought I has found the root cause of the issue but I haven't still found it by now...

As far as I understood, the root cause is somehow linked with the fact that clock signals are treated as standard signals with combinational assigns to one another. This feeling is confirmed by the TODO lying in the treadle source files:

  • https://github.com/freechipsproject/treadle/blob/cf1cfad1f2a94d28c6eab5d536a18724668043ab/src/main/scala/treadle/executable/BlackBoxCycler.scala#L16
  • https://github.com/freechipsproject/treadle/blob/cf1cfad1f2a94d28c6eab5d536a18724668043ab/src/main/scala/treadle/executable/Scheduler.scala#L11-L34 Where registerClocks and triggeredUnassigns are actually unused (for now?).

Considering the current case with:

  • the usual master clock clock
  • a register section driven by the master clock clock
    • regA := A
    • regB := B
  • a blackbox with inputs:
    • inA := regA
    • inB := regB
    • clk := clock

This leads in-the-end to the current issue:

  1. clock posedge
  2. regA driven by A at clock clock update
  3. inA := regA combinational assign
  4. Blackbox event inputChanged("inA", A)
  5. (Then who knows why?) clk := clock combinational assign
  6. Blackbox event clockChange(PositiveEdge, "clk")
  7. regB driven by B at clock clock update
  8. inB := regB combinational assign
  9. Blackbox event inputChanged("inB", B)

Issue: why clock assign is interleaved with register update events? It looks like it should take priority over register changes.

Concerning the VCD events for internal black-box events, it might be useful somehow but it would not be a strong request from my side. As illustrated here, VCD waves do not prove 100% reliable when debugging clock-related issues versus combinational events ordering (everything looks fine on the waves for the current issue).

I cannot invest much more time on this for now by I will stay tuned on treadle updates. Let me know if you try something :)

johnsbrew avatar Aug 19 '19 10:08 johnsbrew

Thanks for putting out the effort there. First comment. The two code references you cite

  • BlackBoxCycler has an old comment, the TODO item there has been done, the scala black box is called in all three cases PosEdge, NegEdge and NoTransition.
  • The unAssigner stuff is left-over from older code and not relevant. I am pretty sure that neither of these is contributing the problem behavior More to come...

chick avatar Aug 19 '19 20:08 chick

As far as clock assignments: The order of clock assignments as ordinary connects could be the problem but I am not sure about that. In Firrtl it is possible to make any single bit wire become a Clock via the asClock primOp. I found that treating them as regular wires and getting the order correct seems to handle all the test cases I had at the time. I am open to the suggestion that I don't have the order correct in all cases. If the problem is more fundamental than that it's going to be harder to fix and I will need more examples.

chick avatar Aug 19 '19 20:08 chick

I will continue poking around, and I certainly understand you are busy. When you get chance can you consider these questions.

  • First off, is there problems here that are not related to black boxes?
  • Is there a fundamental problem with the ordering of the calls (inputChanged, clockChanged, and getOutput) that make it so you cannot write a correct Scala blackbox
  • Is it possible to write a Scala blackbox for any of the orderings, but the ordering is not deterministic so you can't know the implementations strategy.
  • Can you share with me the .v or .sv files that you are trying to do the Scala black box for. I don't think I see it in the issue here. It would be useful for me to try and compare it to the TestCode you attached earlier here.
  • Is there some other case that I'm not getting. (I don't have hardware design sensibilities so I admit that there could be an obvious problem, that I don't see) :-(

chick avatar Aug 19 '19 20:08 chick