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

get_value does not handle signed values properly #265

Open
sgherbst opened this issue Sep 28, 2020 · 1 comment
Open

get_value does not handle signed values properly #265

sgherbst opened this issue Sep 28, 2020 · 1 comment

Comments

@sgherbst
Copy link
Collaborator

Just noticed that when get_value is used in conjunction with SInt types, the values are treated as unsigned. An example is shown below, where the value 214 is returned instead of -42. It appears the reason is that the generated testbench code does not attach the signed attribute to signed signals. However, this only ends up impacting get_value, not expect.

test.py

from pathlib import Path
import fault
import magma as m

class dut(m.Circuit):
    io = m.IO(
        a=m.In(m.SInt[8]),
        y=m.Out(m.SInt[8]),
        clk=m.ClockIn
    )

t = fault.Tester(dut, dut.clk)

t.poke(dut.a, -42)
t.step(2)
y = t.get_value(dut.a)

t.step(2)

t.compile_and_run(
    target='system-verilog',
    simulator='iverilog',
    ext_srcs=[Path('dut.sv').resolve()],
    ext_model_file=True
)

print('y', y.value)

dut.sv

module dut (
    input signed [7:0] a,
    input clk,
    output reg signed [7:0] y
);
    always @(posedge clk) begin
        y <= a;
    end
endmodule
@leonardt
Copy link
Owner

The expect logic calls process_value (

value = self.process_value(action.port, action.value)
) which will convert the expected value from SInt to unsigned for the comparison (
elif isinstance(port, m.SInt) and value < 0:
port_len = len(port)
value = BitVector[port_len](value).as_uint()
value = f"{port_len}'d{value}"
), so the rest of the logic just assumes unsigned.

I looked at the generated TB:

`timescale 1ns/1ns
module dut_tb;
    reg [7:0] a;
    wire [7:0] y;
    reg clk;
    integer get_value_file_file;

    

    dut #(
        
    ) dut (
        .a(a),
        .y(y),
        .clk(clk)
    );

    initial begin
        get_value_file_file = $fopen("/private/tmp/build/get_value_file.txt", "w");
        clk <= 1'b0;
        a <= 8'd214;
        #5 clk ^= 1;
        #5 clk ^= 1;
        $fwrite(get_value_file_file, "%0d\n", a);
        #5 clk ^= 1;
        #5 clk ^= 1;
        $fclose(get_value_file_file);
        #20 $finish;
    end

endmodule

would changing reg [7:0] a to reg signed [7:0] a resolve the issue?

I think the logic could be improved here:

def generate_port_code(self, name, type_, power_args):
to insert the signed modifier for SInt types, but then I think the above expect code (and probably poke code, which I think also uses process_value, so I think just updating process_value might work) would need to be changed to not convert the value to the unsigned representation.

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

No branches or pull requests

2 participants