MikePopoloski / slang

SystemVerilog compiler and language services
MIT License
592 stars 132 forks source link

Warnings to capture binary operations width mismatch #918

Closed hankhsu1996 closed 4 months ago

hankhsu1996 commented 6 months ago

Is your feature request related to a problem? Please describe.

Currently, there doesn't seem to be a warning for binary operation width mismatches, which could be a good feature when using Slang as a linter. For instance, in the following code snippet, there's a typo where b[pipe] is mistakenly written as b, potentially leading to a width mismatch that goes unnoticed:

module Test #(
  parameter int NumPipes = 4,
  parameter int Width = 32
 ) (
  input  logic [NumPipes-1:0][Width-1:0] a, b,
  output logic [NumPipes-1:0][Width-1:0] c
 );
  always_comb begin
    for (int pipe = 0; pipe < NumPipes; pipe++) begin
      // Should be b[pipe] but assume it's a typo
      c[pipe] = a[pipe] + b;
    end
  end
endmodule

Describe the solution you'd like I propose adding a compiler warning for binary operation width mismatches or, more broadly, type mismatches.

warning binary-op-width-mismatch BinaryOpWidthMismatch "binary operation width mismatch: operand '{}' of width {} bits is not the same width as operand '{}' of width {} bits"
jrudess commented 6 months ago

Take a look at -Wextra, -Wwidth-trunc, -Wwidth-expand, -Wport-width-expand, -Wport-width-trunc

slangtest161.sv:11:15: warning: implicit conversion truncates from 128 to 32 bits [-Wwidth-trunc]
      c[pipe] = a[pipe] + b;
              ^ ~~~~~~~~~~~
MikePopoloski commented 6 months ago

It is true that there is no specific warning for binary operator width mismatch, only warnings about conversions on assignments. More generally this relates to the weird Verilog "feature" of propagating the type of the lhs back down to the rhs, through binary operators. slang doesn't currently warn for propagated type conversions, because I'm worried there will be many such warnings. Even the "canonical" example of the bit width feature below would warn for the 17-bit add; maybe you want that and maybe you don't?

logic [15:0] a, b; // 16-bit variables
logic [15:0] sumA; // 16-bit variable
logic [16:0] sumB; // 17-bit variable
sumA = a + b; // expression evaluates using 16 bits
sumB = a + b; // expression evaluates using 17 bits

I could always add it as a separate warning class but it probably requires some thought to make it actually useful.

hankhsu1996 commented 6 months ago

@jrudess, I have tried -Wextra. Since it only checks for width mismatches in assignments, we cannot always rely on it to capture mismatches in RHS. For example, in the code c[pipe] = a[pipe] + b[pipe][0];, the width of b[pipe][0] is promoted to match the width of a[pipe], resulting in the RHS and LHS having the same width, and no warning is generated.

Even in the case of c[pipe] = a[pipe] + b;, I would argue that the cause of the assignment width mismatch is the mismatch in the binary operation, so a warning for binary operations could help identify the cause faster.

@MikePopoloski, yes, that is exactly what I want. Although it's a 'feature' in SystemVerilog, I would like to have some sort of strict mode that does not allow the expansion, truncation, or promotion of widths, nor the promotion of integrals to reals, unless explicitly specified.

MikePopoloski commented 4 months ago

b1354ad12f8213d0f429b66d8d84c3602950591e adds -Warith-op-mismatch, -Wbitwise-op-mismatch, -Wcomparison-mismatch, and -Wsign-compare