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

Warnings to capture binary operations width mismatch #918

Closed
hankhsu1996 opened this issue Mar 23, 2024 · 4 comments
Closed

Warnings to capture binary operations width mismatch #918

hankhsu1996 opened this issue Mar 23, 2024 · 4 comments
Labels

Comments

@hankhsu1996
Copy link

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
Copy link
Contributor

jrudess commented Mar 26, 2024

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
Copy link
Owner

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
Copy link
Author

@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
Copy link
Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants