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

Cannot write to top-level tristate ports #1373

Open
veripoolbot opened this issue Dec 5, 2018 · 2 comments
Open

Cannot write to top-level tristate ports #1373

veripoolbot opened this issue Dec 5, 2018 · 2 comments

Comments

@veripoolbot
Copy link

@veripoolbot veripoolbot commented Dec 5, 2018


Author Name: Stephen Richardson
Original Redmine Issue: 1373 from https://www.veripool.org


DESCRIPTION of the bug:
Top level has bidirectional port bus1 and direction bit en1. When
en1==0, bus1 is used as an input port, else it is an output port.
When bus1 is an output it reads an internal id code (13333).
A result port emits the value (bus1+10000).

Then test harness sets en1==1 for five clock cycles, hoping to see
result 23333, which succeeds.

Then test harness sets en1==0 and writes sequential values (1,2,3,4,5) to
bus1, hoping to see result (10001,10002,10003,10004,10005).
This fails, we see only zeroes on bus1 (below).
Why?

Vtop
  en1=1=>bus1=OUTPUT, bus1 should equal internal id 13333
     i=1 en1=1 bus1=13333 res=23333
     i=2 en1=1 bus1=13333 res=23333
     i=3 en1=1 bus1=13333 res=23333
     i=4 en1=1 bus1=13333 res=23333
     i=5 en1=1 bus1=13333 res=23333
     OKAY

  en1=0=>bus1=INPUT, should count to five
     i=1 en1=0 bus1=00000 res=10000
     i=2 en1=0 bus1=00000 res=10000
     i=3 en1=0 bus1=00000 res=10000
     i=4 en1=0 bus1=00000 res=10000
     i=5 en1=0 bus1=00000 res=10000
     FAIL!


MANIFESTATION of the bug:

Need: top.v, harness.cpp (see below)

Do: ./build_and_run.sh

...which does something like this:

     verilator -I.  --trace -Mdir . \
       -CFLAGS '-std=c++11 -DTRACE' -Wno-fatal --cc top \
       --exe harness.cpp --top-module top \
       || exit 13
     make -j -f Vtop.mk Vtop || exit 13
     Vtop


NOTES on the bug:

This combo (below), in Vtop.cpp, overwrites values trying to come in on port
"inout bus1" via the harness.

// NOTE THIS OVERWRITES HARNESS VALUE OF bus1 WITH ZERO!
VL_INLINE_OPT void Vtop::_combo__TOP__1(Vtop__Syms* __restrict vlSymsp) {
     VL_DEBUG_IF(VL_PRINTF("    Vtop::_combo__TOP__1\n"); );
     Vtop* __restrict vlTOPp VL_ATTR_UNUSED = vlSymsp->TOPp;
     // Body
     vlTOPp->bus1 = (0xffffU & ((((IData)(vlTOPp->en1)
                                   ? 0x3415U : 0U) & 
                                 ((IData)(vlTOPp->en1)
                                   ? 0xffffffffU : 0xffff0000U)) 
                                & ((IData)(vlTOPp->en1)
                                    ? 0xffffffffU : 0xffff0000U)));
}
...
void Vtop::_eval(Vtop__Syms* __restrict vlSymsp) {
     VL_DEBUG_IF(VL_PRINTF("    Vtop::_eval\n"); );
     Vtop* __restrict vlTOPp VL_ATTR_UNUSED = vlSymsp->TOPp;
     // Body

     // NOTE THIS (combo1) OVERWRITES HARNESS VALUE OF bus1 WITH ZERO!
     vlTOPp->_combo__TOP__1(vlSymsp);
     vlTOPp->_combo__TOP__3(vlSymsp);
}

harness.cpp

#include "Vtop.h"
#include "verilated.h"
#include <iostream>
#include "stdint.h"
#include <fstream>
#include "printf.h"
#include "verilated_vcd_c.h"

int main(int argc, char **argv) {
     Verilated::commandArgs(argc, argv);
     Vtop* top = new Vtop;
     
     Verilated::traceEverOn(true);
     VerilatedVcdC* tfp = new VerilatedVcdC;
     top->trace(tfp, 99); // What is 99?  I don't know!  FIXME
     top->clk = 0;

     //tfp->open("/nobackup/steveri/github/CGRAGenerator/verilator/generator_z_tb/tmp.vcd");
     tfp->open("tmp.vcd");
     uint32_t time_step = 0;

         // bus1 as output; should read 13333
         std::cout << "  bus1=OUTPUT, bus1 should equal internal id 13333" << std::endl;
         top->en1 = 1; // bus1 = OUTPUT

         for (int i = 1; i <= 5; i++) {
             top->clk ^= 1; top->eval(); tfp->dump(time_step); time_step++;
             printf("    i=%d en1=%1d bus1=%05d res=%05d\n",
                         i, top->en1, top->bus1, top->result);
         }
         printf("\n");

         ////////////////////////////////////////////////////////////////////////
         // bus1 = INPUT, bus1 should count to five
         std::cout << "  bus1=INPUT, should count to five" << std::endl;
         top->en1 = 0; // bus1 = INPUT

         for (int i = 1; i <= 5; i++) {
             top->bus1 = i;
             top->clk ^= 1; top->eval(); tfp->dump(time_step); time_step++;
             printf("    i=%d en1=%1d bus1=%05d res=%05d\n",
                         i, top->en1, top->bus1, top->result);
         }

     tfp->close();
}

top.v

module top (
  inout  [15:0] bus1,
  input en1, // Set to 0 for input, 1 for output

  input clk,
  output [15:0] result
);
tri  [15:0] bus1;
wire en1;
wire clk;
wire [15:0] result;

wire [15:0] bus1_in;

// Bidirectional bus 1
// If en1==0 can read id '13333' from bus1
// else bus1 = whatever harness sets it to
assign bus1 = en1 ? 13333 : 16'bz;

assign result = bus1 + 10000;

/*
always @(bus1 or en1 or clk) begin
  $display("        %m: en1=%0b bus1=%d", en1, bus1);
end
*/

endmodule

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 6, 2018


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-12-06T01:38:43Z


Verilator doesn't currently support tristate resolution that is a mix of external and internal drivers.

What it should be doing is what it does for internal cell tristate resolution, that is make a bus1, bus1__en, and bus1__out primary signals, your code then needs to drive bus1 and bus1__en appropriately, and Verilator will compute the resolving equation and output bus1__out.

Would you be willing to take a look at fixing this? The code that resolves tristates is in src/V3Tristate.cpp.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Dec 6, 2018


Original Redmine Comment
Author Name: Stephen Richardson
Original Date: 2018-12-06T17:10:15Z


Would you be willing to take a look at fixing this?

Hm I am not currently set up to compile verilator from source, but I suppose I could put it on my to-do list (i.e. may or may not get done in finite time).

FYI our current workaround is to separate each top-level IO pad "pad_i" into two separate pads "pad_i_in" and "pad_i_out" for verilator simulation, similar to your suggestion; then for the actual chip we reunite the pads and use ncsim for the final verification. This tristate bug is the last/only thing that keeps us from using the same verilog code base for both verilator and ncsim...

FYA this is our project (link below). Extensive verilator checks run on the entire chip at each github checkin (via travis):
https://github.com/StanfordAHA/CGRAGenerator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.