steveicarus / iverilog

Icarus Verilog
https://steveicarus.github.io/iverilog/
GNU General Public License v2.0
2.86k stars 530 forks source link

no warning for scaler being access with indexed part select. No warning for double assignment of combiniational logic #1142

Closed Adivinedude closed 4 months ago

Adivinedude commented 4 months ago

yall are doing great work. sorry to keep popping up with strange and unusual issues. I found two possible warning/errors that are not being generated.

Im trying to create a module that will scale to a latency parameter. For all but .LATENCY('d0), the vectors will have a width greater than 1. So I declare the vectors with [MSB:LSB]. Latency zero is a valid option, so the vector declaration becomes [0:0]. This is causing some very strange issues when simulating with IVerilog.

Using the module math_combinational directly. "wire result" returns the correct value. but when I pass a wire vector to math_combinational through module math_lfmr, strange things happen. If the == operation results in FALSE0 then the results_two is set to false, but if the operation results in TRUE1, then 'results_two' is set to unknown 1'bx.

the following test bench is stripped down code that will reproduce the behavior so it looks a little weird. I think its call indexed part select, that I am using to select the piece of the vector I want to operate on. But the vector is being defined as a scaler(size of 1) wire [0:0] vector, which should generate an error message when being accessed as vector[idx] syntax

Its was the strangest thing. I get the `1'bx' result, but why isn't it always 1'bx, and the part about piping it through another module, to me shouldn't have an effect. anyone see what Im doing wrong?

yep!! just needed a good nights sleep and some fresh eyes. Glad I waited to post this, found my error, so line 49 is my problem. I assign the wire vector, then use it as an output port in a module. so basically it gets assigned twice. in one instance it is assign to zero, in another, it could be 0 or 1, but the conflicting assign statements result in 1'bx. a warning message would be appropriate. again, thank yall for all the hard work, this is a great program yall have made.

`default_nettype none

//`define CAUSE_ERROR
//`define FIX_UNDEFINED_BEHAVIOR

`ifndef CAUSE_ERROR
    `define VECTOR_DECLARATION [0:0]    // creates undefined behavior
`else
    `define VECTOR_DECLARATION
`endif

`ifndef FIX_UNDEFINED_BEHAVIOR
    `define DEFINE_PORT w_CMP_EQ_CHAIN
`else
    `define DEFINE_PORT
`endif

module math_combinational

    (   clk, I1, I3,
        cmp_eq, cmp_eq_carry_out,
    );
        input   wire    clk;
        input   wire    [1:0] I1;
        input   wire    [1:0] I3;

    genvar idx;

//cmp_eq
    output  wire                 cmp_eq;
    output  wire    `VECTOR_DECLARATION    cmp_eq_carry_out;
    assign cmp_eq = cmp_eq_carry_out`VECTOR_DECLARATION;
    for( idx = 0; idx <= 0; idx = idx + 1 ) begin : CMP_EQ_base_loop
        assign cmp_eq_carry_out[idx] = I1[idx*2+:2] == I3[idx*2+:2];
    end
endmodule

module math_lfmr
    (
        input   wire            clk,
        input   wire            rst,
        input   wire    [1:0]   I1,
        input   wire    [1:0]   I3,
        output  wire            cmp_eq
    );

    wire    `VECTOR_DECLARATION  w_CMP_EQ_CHAIN;
    assign w_CMP_EQ_CHAIN`VECTOR_DECLARATION = 0;

    math_combinational ALU_LOGIC
    (
        .clk(clk),
        .I1(I1),
        .I3(I3),
        .cmp_eq(cmp_eq),   
        .cmp_eq_carry_out(`DEFINE_PORT)
    );    

endmodule

module bug_report();
    reg clk = 0;
    always #1 clk <= ~clk;
    initial #20 $finish;

    reg [1:0]I1 = 1;
    reg [1:0]I3 = 0;

    wire result;
    wire results_two;
    always @(posedge clk) begin
        I1 <= I1 + 1'b1;
        $display("I1:%b result %b results_two %b", I1, result, results_two);
    end

    math_combinational uut(
        .clk(clk),
        .I1(I1),
        .I3(I3),
        .cmp_eq(result),
        .cmp_eq_carry_out()
    );
    math_lfmr uut_two(
        .clk(clk),
        .I1(I1),
        .I3(I3),
        .cmp_eq(results_two)
    );
endmodule
I1:01 result 0 results_two 0
I1:10 result 0 results_two 0
I1:11 result 0 results_two 0
I1:00 result 1 results_two x
I1:01 result 0 results_two 0
I1:10 result 0 results_two 0
I1:11 result 0 results_two 0
I1:00 result 1 results_two x
I1:01 result 0 results_two 0
I1:10 result 0 results_two 0
caryr commented 4 months ago

A couple comments:

wire [0:0] is a one element vector so a part select of any kind is perfectly valid.

Wires can have multiple drives and have defined resolution functions regarding how to resolve any driver mismatches. A uwire can only have a single driver and will complain when multiple drivers are used. In SystemVerilog you can also assign to a variable using a single driver so if you goal is to create a warning when you have multiple drivers than you should use either a uwire or a variable.

Given all this is there still something you expect to give different results?

Adivinedude commented 4 months ago

I was completely unaware of a uwire in verilog. Thank you