Skip to content

ImportVerilog lowers ConversionExpression with the wrong signedness #10234

@kaituo-crypto

Description

@kaituo-crypto

ImportVerilog lowers ConversionExpression with the wrong signedness

Summary

ImportVerilog appears to lower a slang::ast::ConversionExpression using the operand's signedness instead of the conversion node's target semantics.

In the reproducer below, the relevant AST conversion target is logic [8:0] (unsigned), but it is materialized as moore.sext instead of zero-extension.

Reproducer

module M(
    input  logic [7:0]        in1,
    input  logic signed [7:0] in2,
    output logic signed [8:0] mux_out
);
  logic [7:0] unsigned_src;
  logic signed [7:0] signed_src;

  always_comb signed_src = in2 >>> 1;
  always_comb unsigned_src = in1 >> 1;
  always_comb mux_out = signed_src + unsigned_src;
endmodule

Reproduce with:

circt-verilog --ir-moore minimal_testcase.sv

AST evidence

The relevant Slang AST node already shows an unsigned conversion target:

{
  "kind": "Conversion",
  "type": "logic[8:0]",
  "operand": {
    "kind": "NamedValue",
    "type": "logic signed[7:0]",
    "symbol": "signed_src"
  }
}

This is the conversion that ImportVerilog lowers to moore.sext .

Expected behavior

The widening conversion from logic signed [7:0] to logic [8:0] should be zero-extension, since the conversion target is unsigned.

Actual behavior

The lowered Moore IR contains:

%3 = moore.sext %2 : l8 -> l9

After a local fix, this becomes:

%3 = moore.zext %2 : l8 -> l9

Evidence

Current Moore IR:

module {
  moore.module @M(in %in1 : !moore.l8, in %in2 : !moore.l8, out mux_out : !moore.l9) {
    %0 = moore.constant 1 : i32
    %in1_0 = moore.variable name "in1" : <l8>
    %in2_1 = moore.variable name "in2" : <l8>
    %mux_out = moore.variable : <l9>
    %unsigned_src = moore.variable : <l8>
    %signed_src = moore.variable : <l8>
    moore.procedure always_comb {
      %2 = moore.read %in2_1 : <l8>
      %3 = moore.ashr %2, %0 : l8, i32
      moore.blocking_assign %signed_src, %3 : l8
      moore.return
    }
    moore.procedure always_comb {
      %2 = moore.read %in1_0 : <l8>
      %3 = moore.shr %2, %0 : l8, i32
      moore.blocking_assign %unsigned_src, %3 : l8
      moore.return
    }
    moore.procedure always_comb {
      %2 = moore.read %signed_src : <l8>
      %3 = moore.sext %2 : l8 -> l9
      %4 = moore.read %unsigned_src : <l8>
      %5 = moore.zext %4 : l8 -> l9
      %6 = moore.add %3, %5 : l9
      moore.blocking_assign %mux_out, %6 : l9
      moore.return
    }
    moore.assign %in1_0, %in1 : l8
    moore.assign %in2_1, %in2 : l8
    %1 = moore.read %mux_out : <l9>
    moore.output %1 : !moore.l9
  }
}

The problematic conversion is the signed_src widening:

%3 = moore.sext %2 : l8 -> l9

Suspected location

  • lib/Conversion/ImportVerilog/Expressions.cpp
  • visit(const slang::ast::ConversionExpression &expr)
  • Context::convertRvalueExpression(...)
  • Context::materializeConversion(...)

The signedness passed into materializeConversion appears to come from the operand's type instead of the conversion node's target semantics.

Notes

  • Initial report: @m2kar
  • Analysis and reproducer: @kaituo-crypto

Tested with:

  • firtool-1.139.0

Metadata

Metadata

Assignees

No one assigned

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions