UPGRADE YOUR BROWSER

We have detected your current browser version is not the latest one. Xilinx.com uses the latest web technologies to bring you the best online experience possible. Please upgrade to a Xilinx.com supported browser:Chrome, Firefox, Internet Explorer 11, Safari. Thank you!

cancel
Showing results for 
Search instead for 
Did you mean: 
Scholar helmutforren
Scholar
1,174 Views
Registered: ‎06-23-2014

Another simulation bug w.r.t. SystemVerilog structures

I'm using Vivado 2017.1.  I keep running into problems that simply MUST be bugs in Vivado, for either simulation or implementation, relating to my detailed use of structures.  Yet again I've run into one.  I would LOVE to learn what I'm doing wrong, or else how to keep the bug from biting me.  I don't think I'm doing anything wrong, however.

 

Note that I can't provide the full project.  Furthermore, I can't boil the project down to merely the offensive code, because it will likely miraculously start working again.  All I can do is provide the germane source code at face value and ask, "is there something specifically incorrect with this usage?  Or is it indeed a bug in Vivado simulation?"

 

Specifically, look at the three yellow traces below, also identified by three red arrows.  I assert that these three traces should be very nearly IDENTICAL.  However, the first two are DRASTICALLY DIFFERENT from the third, which is the origin of the info.

is_control_bug Waveform 20170829 (low res, low quality).jpg

 

In detail, that "control_flag" is buried in data coming from a FIFO.  All I'm trying to do is perform an "if" on its value.  However, my simulation isn't behaving as it should.  So I create a wire called "fm_is_control" and assign to it "control_flag".  So "fm_is_control" should look EXACTLY the same as "control_flag".  But it doesn't.  (In truth, it did an hour ago, but when I added "fm_is_control_buffered", it STOPPED looking exactly the same.  The current problem is akin to one I've hit before, so I'm pausing here to ask, once more, what's up with this?)

 

This really seems to be a problem where Vivado does NOT handle structures correctly.  Note that previously it was a modport problem.  I believe it was @vemulad Deepika who helped me get past the prior problem by using modports.  But now I'm using modports and still have new problems.

 

I will paste some germane code segments.  In theory, this should be enough to prove what I am doing wrong or that Vivado has a bug.  Just in case, I'll post most of the code after that.  Unfoturnately, I'm not allowed to post all of the code.

 

Note that I'm using a two-always-block state machine coding style with one clocked block (using NBA's) and one combinatorial block (using BA's).  Aside from the periodic Vivado bug, I find this develops very rapidly.  I got the idea from outside research, not on my own.

 

So here is the germane code:

`include "MEM_read_fifo.include.sv"

module fps60_Average (
        input clk,
        input rst,
        i_CL_fifo.pop Input_FIFO_pop,   // Input to this module
        i_CL_fifo.push Output_FIFO_push,  // Output from this module, 60 fps video
        i_MEM_wr_fifo.push fps960_to_MEM_fifo_push,
        i_MEM_wr_fifo.pop fps960_from_MEM_fifo_pop
    );
	
    ...

    always @(posedge clk) begin
        if (rst) begin
            PROCFM_state <= IDLE_procfm;
        end else begin
            PROCFM_state <= next_PROCFM_STATE;
        end
    end

    wire fm_is_control = fps960_from_MEM_fifo_pop.pop_data.control_word_array.control_words[7].control_flag;
    reg fm_is_control_buffered;

    always @* begin
	...
	case (PROCFM_state) 
	...
        GET_DATA_procfm: begin
	    if (!fps960_from_MEM_fifo_pop.empty) begin    // If input not empty 
		if (!fm_is_control_buffered) begin
	            // Do this
		    ...
		end else begin
		    // Do that
		    ...
		end
	    end
	end
	endcase
	...
    end
	
    ...
	
endmodule

and here is the structure definition:

typedef struct packed {     // 65 BITS
    logic control_flag;   //      [64:64] Control Flag
    logic [1:0] writeflag;  //    [63:62] If 1 then write.  If 0 then read.  If 2 then pass through (neither read nor write) 
    logic [27:0] address;  //     [61:34] Memory Address
    logic [4:0] length;  //       [33:29] Memory Length (in BURSTS, 8x64bits.  This corresonds to 32 pixels)
    logic [3:0] destination;  //  [28:25] (Use `DEST Defines) Used for routing packets through demultiplexers/distributors
    logic [2:0] unused;   //      [24:22] Unused
    logic [2:0] laneidx;  //      [21:19] Communications Lane from which this packet came, numbered 0-7 for lanes 1-8.
    logic [7:0] row;      //      [18:11] Row within this lane of first data in packet
    logic [7:0] col;      //      [10:03] Column within this lane of first data in packet
    logic [2:0] frame_format;//   [02:00] Shorthand for number of rows, cols, and blanking
} t_COMMON_control_word; // named structure type

typedef struct packed {
    logic control_flag;     //      [64:64] Control Flag should be 0 for data
    logic [63:0] data64;      //      [63:00] Data
} t_MEM_rdwr_fifo_data_word; // named structure type

typedef struct packed {
    t_COMMON_control_word [0:7] control_words; // Run from 0:7 so we see [0] on waveforme easier, so see single control_flag
} t_MEM_wr_fifo_control_word_array; // named structure type

typedef struct packed {
    t_MEM_rdwr_fifo_data_word [0:7] data_words; // Run from 0:7 because control words do
} t_MEM_rdwr_fifo_data_word_array; // named structure type

typedef union packed {
    t_MEM_wr_fifo_control_word_array control_word_array;
    t_MEM_rdwr_fifo_data_word_array data_word_array;
} t_MEM_wr_fifo_word;

// New modport style:
interface i_MEM_wr_fifo;    
    t_MEM_wr_fifo_word push_data;
    logic wr_en;
    logic full;
    logic prog_full;

    t_MEM_wr_fifo_word pop_data;
    logic rd_en;
    logic empty;
    
    modport push (
        input full,
        input prog_full,
        output wr_en,
        output push_data
    );
    
    modport pop (
        input empty,
        output rd_en,
        input pop_data
    );
endinterface

Finally, here's all the code surrounding this state machine.  It's not the WHOLE module, which as three state machines, and it's now the WHOLE project, which has many modules.

`include "MEM_read_fifo.include.sv"

module fps60_Average (
        input clk,
        input rst,
        i_CL_fifo.pop Input_FIFO_pop,   // Input to this module
        i_CL_fifo.push Output_FIFO_push,  // Output from this module, 60 fps video
        i_MEM_wr_fifo.push fps960_to_MEM_fifo_push,
        i_MEM_wr_fifo.pop fps960_from_MEM_fifo_pop
    );
	
	...

    
    //=================================================================================================================
    // Advance Declarations
    //=================================================================================================================

    `define START_ADDRESS   (32768+1536)

    //=================================================================================================================
    // FPS60_PROCESSOR INPUT FROM MEMORY - State Machine Declarations
    //=================================================================================================================

    typedef enum bit [3:0] {
        IDLE_procfm,
        GET_CTRL_procfm,
        GET_DATA_procfm,
        DISTRIBUTE_DATA_procfm
    } t_PROCFM_STATE; 
    
    t_PROCFM_STATE PROCFM_state, next_PROCFM_STATE;
    
    reg [3:0] fifoword_counter_fm, next_fifoword_counter_fm;  // Used to count incoming 65-bit fifowords, that get split into groups of 6 in order to fill 520-bit memory fifowrd 

//    reg [27:0] address_fm, next_address_fm;
    
    reg [20:0] distributor_pixel1, distributor_pixel2, distributor_pixel3, distributor_pixel4;
    
    reg fm_empty, next_fm_empty;   // Set TRUE if data from this state machine is empty.  If FALSE, then data from this state machine is ready to use
    reg fm_rd_en;   // Set TRUE by client after using data from this state machine.  Will cause fm_empty to go TRUE until data is replaced
    reg fm_rd_flush;    // Set TRUE by client to instruct this state machine to flush everything in receive fifo

    wire fm_is_control = fps960_from_MEM_fifo_pop.pop_data.control_word_array.control_words[7].control_flag;
    reg fm_is_control_buffered;

    //=================================================================================================================
    // FPS60_PROCESSOR INPUT FROM MEMORY - State Machine 
    //=================================================================================================================
    
    always @(posedge clk) begin
        if (rst) begin
            PROCFM_state <= IDLE_procfm;
            fifoword_counter_fm <= 0;
//            address_fm <= `START_ADDRESS;
            fm_empty <= 1;
        end else begin
            PROCFM_state <= next_PROCFM_STATE;
            fifoword_counter_fm <= next_fifoword_counter_fm;
//            address_fm <= next_address_fm;
            fm_empty <= next_fm_empty;
        end
    end
    
    always @* begin
        next_PROCFM_STATE = PROCFM_state;       // Default remain in current state, unless overridden below
        next_fifoword_counter_fm = fifoword_counter_fm;   // Default remain the same
//        next_address_fm = address_fm;             // Default remain the same

        fps960_from_MEM_fifo_pop.rd_en = 0;   // Default to NOT read (consume) unless overridden below
        
        if (fm_rd_en) begin
            next_fm_empty = 1;                      // If client is consuming our output, mark our output empty
        end else begin
            next_fm_empty = fm_empty;               // Default remain the same
        end
        
        fm_is_control_buffered = fm_is_control; // Trying to get "always @* begin" to execute in simulation when fm_is_control changes.  This buffering adds tiny delay
        
        if (fm_rd_flush) begin
            // Override state machine and simply read, read, read to flush input 

            // WARNING: Doesn't wait or anything to confirm that flush is complete
            // WARNING: Doesn't wait or anything to confirm that flush is complete
            // WARNING: Doesn't wait or anything to confirm that flush is complete

            fps960_from_MEM_fifo_pop.rd_en = 1;  // Read (consume) the input
        end else begin
            case (PROCFM_state) 
            IDLE_procfm: begin
                next_PROCFM_STATE = GET_CTRL_procfm;    
            end
            GET_CTRL_procfm: begin
                if (!fps960_from_MEM_fifo_pop.empty) begin    // If input not empty (and note we're leaving rd_en default 1 this clock cycle)
                    if (fm_is_control_buffered) begin
                        // If this is the control we expect
    
                        fps960_from_MEM_fifo_pop.rd_en = 1;  // Read (consume) the input, but ONLY now that we're ready to process this control header
                            
                        next_PROCFM_STATE = GET_DATA_procfm; // But ONLY now that we're ready to process this control header
    
                    end else begin
                        // Otherwise this is data when we were expecting a control
                        // Just burn it.  Set rd_en==1 and stay in this state
                        fps960_from_MEM_fifo_pop.rd_en = 1;  // Read (consume) the input
                    end
                end
            end
            GET_DATA_procfm: begin
                if (!fps960_from_MEM_fifo_pop.empty) begin    // If input not empty 
                    if (!fm_is_control_buffered) begin
                        // If this is data as we expect
    
                        fps960_from_MEM_fifo_pop.rd_en = 1;  // Read (consume) the input, but ONLY now that we're ready to process this control header
                        
                        next_fifoword_counter_fm = 0;       // Reset counter for distributor
                        next_PROCFM_STATE = DISTRIBUTE_DATA_procfm; // But ONLY now that we're ready to process this control header
                        
                    end else begin
                        // But if this is a control, then don't read it but go to GET_CTRL state
//bug?//                next_PROCFM_STATE = GET_CTRL_procfm;  
                    end
                end
            end
            DISTRIBUTE_DATA_procfm: begin
                if (fm_empty) begin
                    // Then room for more data output
                    // NOTE: Before getting to this state, the fact has already been established that fps960_from_MEM_fifo_pop has valid data (is not empty and not a control)
                    
                    // NOTE: We have packed three pixels of 21 bits each into 63 bits of the memory fifoword's 64 bits.  Now extract them appropriately.
                    case (fifoword_counter_fm)
                    0: begin
                        distributor_pixel1 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[0].data64[62:42];
                        distributor_pixel2 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[0].data64[41:21];
                        distributor_pixel3 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[0].data64[20:0];
                        distributor_pixel4 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[1].data64[62:42];
                    end
                    1: begin
                        distributor_pixel1 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[1].data64[41:21];
                        distributor_pixel2 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[1].data64[20:0];
                        distributor_pixel3 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[2].data64[62:42];
                        distributor_pixel4 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[2].data64[41:21];
                        
                    end
                    2: begin
                        distributor_pixel1 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[2].data64[20:0];
                        distributor_pixel2 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[3].data64[62:42];
                        distributor_pixel3 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[3].data64[41:21];
                        distributor_pixel4 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[3].data64[20:0];
                    end
                    3: begin
                        distributor_pixel1 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[4].data64[62:42];
                        distributor_pixel2 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[4].data64[41:21];
                        distributor_pixel3 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[4].data64[20:0];
                        distributor_pixel4 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[5].data64[62:42];
                    end
                    4: begin
                        distributor_pixel1 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[5].data64[41:21];
                        distributor_pixel2 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[5].data64[20:0];
                        distributor_pixel3 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[6].data64[62:42];
                        distributor_pixel4 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[6].data64[41:21];
                    end
                    5: begin
                        distributor_pixel1 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[6].data64[20:0];
                        distributor_pixel2 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[7].data64[62:42];
                        distributor_pixel3 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[7].data64[41:21];
                        distributor_pixel4 = fps960_from_MEM_fifo_pop.pop_data.data_word_array.data_words[7].data64[20:0];
    
                        fps960_from_MEM_fifo_pop.rd_en = 1;  // Read (consume) the input
    
                        next_PROCFM_STATE = GET_DATA_procfm; // Go get more data
                    end
                    endcase
    
                    next_fm_empty = 0;  // Indicate that we have output available for client
                    
                    next_fifoword_counter_fm = fifoword_counter_fm + 1; // Increment to next position.  If goes too far, we'll be leaving this state anyway
                end // Otherwise no more room for output.  Just stay in this state until there is
            end
            default: begin
                next_PROCFM_STATE = IDLE_procfm;   // If unknown state, go to idle state
            end
            endcase
        end
    end
	
	...
	
endmodule

 

 

================================= EDIT #1 ==================================================

 

I added the following signal definitions and code:

    
    t_MEM_wr_fifo_word local_pop_data = fps960_from_MEM_fifo_pop.pop_data;
    t_MEM_wr_fifo_control_word_array local_control_word_array = fps960_from_MEM_fifo_pop.pop_data.control_word_array;
    t_COMMON_control_word local_control_words_7 = fps960_from_MEM_fifo_pop.pop_data.control_word_array.control_words[7];
    logic local_control_flag = fps960_from_MEM_fifo_pop.pop_data.control_word_array.control_words[7].control_flag;
    
    t_MEM_wr_fifo_word alocal_pop_data;
    t_MEM_wr_fifo_control_word_array alocal_control_word_array;
    t_COMMON_control_word alocal_control_words_7;
    logic alocal_control_flag;

// inside always @* begin I also added:

        alocal_pop_data = fps960_from_MEM_fifo_pop.pop_data;
        alocal_control_word_array = fps960_from_MEM_fifo_pop.pop_data.control_word_array;
        alocal_control_words_7 = fps960_from_MEM_fifo_pop.pop_data.control_word_array.control_words[7];
        alocal_control_flag = fps960_from_MEM_fifo_pop.pop_data.control_word_array.control_words[7].control_flag;

So it seems none of the assignments to local signals worked.  I guess because I can't put the word "wire" in front of them.  The simulation shows X values.

 

But the alocal signals worked -- partially.  The first three worked.  The last one did NOT work.  It's the last one that I need.  (Note I used "logic" rather than "reg" in order to be consistent.  I used "reg" originally, anyway.)

 

Here's the new waveform:

is_control_bug Waveform 201708291730 (low res, low quality).jpg

 

================================= EDIT #2 ==================================================

 

Since all but my last alocal was working, I subsequently created an rlocal signal to be blocking-assigned from the next-to-last and working alocal.  THIS works... for no darned good reason that I can see.

 

Here's the extra code:

    reg rlocal_control_flag;

// Then inside the "always @* begin" I added

    rlocal_control_flag = alocal_control_words_7.control_flag;

Then lo and behold, rlocal_control_flag correctly looks like the very original fps960_from_MEM_fifo_pop/.../control_flag.

is_control_bug Waveform 201708291743 (low res, low quality).jpg

 

 Perhaps Deepika ( @vemulad ) has a thought here?

 

 

 

0 Kudos
2 Replies
Xilinx Employee
Xilinx Employee
1,127 Views
Registered: ‎10-24-2013

Re: Another simulation bug w.r.t. SystemVerilog structures

Hi @helmutforren

Do you see the same behavior in Vivado 2017.2?

Thanks,Vijay
--------------------------------------------------------------------------------------------
Please mark the post as an answer "Accept as solution" in case it helped resolve your query.
Give kudos in case a post in case it guided to the solution.
0 Kudos
Scholar helmutforren
Scholar
1,110 Views
Registered: ‎06-23-2014

Re: Another simulation bug w.r.t. SystemVerilog structures

Being still on deadline, I've been afraid to upgrade.  Do you know how smooth or roughly upgrading to 2017.2 would be?  Do you know if I can run both versions at the same time?

 

EDIT: Oh, yes.  I remember now.  My major fear is about licensing.  No matter what, it always seems to be a big problem any time I change anything.

0 Kudos