MikePopoloski / slang

SystemVerilog compiler and language services
MIT License
620 stars 138 forks source link

width-port-trunc not flagging for 32-bit expression in port list #896

Closed jrudess closed 8 months ago

jrudess commented 8 months ago

Found this one through a difference in warnings reported by Slang vs VCS. Slang behavior may be the desired behavior.

In the m1 module connection, the a1 variable has the same width as the port, but the entire expression should be treated as a 32-bit value. The port-width-trunc warning doesn't flag for this case.

In the m2 module connection, the a2 variable is larger, but the entire expression is still 32 bits. In this case the warning does get flagged and it does mention that the truncation is from 32->4.

Should this warning fire for the m1 port connection?

slangtest152.sv:16:12: warning: implicit conversion of port connection truncates from 32 to 4 bits [-Wport-width-trunc]
        .a(a2 & (2**USED_A - 1))
           ^~~~~~~~~~~~~~~~~~~~
module M(
    input bit [3:0] a
);
endmodule

module top;
    localparam USED_A = 2;
    bit [3:0] a1;
    bit [8:0] a2;

    M m1(
        .a(a1 & (2**USED_A - 1))
    );

    M m2(
        .a(a2 & (2**USED_A - 1))
    );
endmodule
MikePopoloski commented 8 months ago

This is intentional. Before issuing a size conversion warning, slang computes an "effective width" of the expression, taking into account the actual sizes of any constants involved. In this case that means a1 & (2**USED_A - 1) is computed as an effective width of 4, so no warning. The rationale is to reduce false positive conversion warnings when they should be known to be 100% safe.

One thing slang does not currently do but could would be to count a bitwise-AND against a constant as effectively a width-reducing operation, since you guarantee that the top bits of such an operation are always zero. That would also silence the warning on the second case.

jrudess commented 8 months ago

Should the warning for the m2 port connection report the effective-width instead of the expression width? I.e. warn about an 8->4 truncation instead of 32->4?

MikePopoloski commented 8 months ago

Hmm, perhaps. It would be a trivial change to make. If it reduces instead of adding confusion then it makes sense to do so.

jrudess commented 8 months ago

Actually, it really doesn't matter since the solution to resolve the warning is the same regardless of which input width is reported. Either way the user has a warning to fix :-)

I only submitted this since it's one of those cases where Slang was warning/error free, but then the code failed to compile because the VCS warning had been elevated to an error by the project-team. But I do prefer the Slang behavior of reducing false-positives.