Frontend/AST: signed assign to indexed part-select

[phsauter]

Version

Yosys 0.36+13 (git sha1 2858c33, clang 10.0.0-4ubuntu1 -fPIC -Os)

On which OS did this happen?

Linux

Reproduction Steps

The zip contains the source RTL, yosys output and a testbench.
sign_part_assign.zip

Execute the commands:

read_verilog sign_part_assign.v; proc; clean -purge; write_verilog -noattr sign_part_assign.yosys.v;

Compare the output data_o using the testbench or compare by inspection (its only a few lines to think through).

From inspecting the debug output from read_verilog -debug I expect the problem to be in the AST simplification step.

Expected Behavior

Source RTL:

always @(*) begin
    data_ref_o[offset_i+:4] = 4'b1111;
    data_o[offset_i+:4]     = 1'sb1;
end

The RHS of the second expression should be sign extended to match LHS width as per Sec 5.5.3 of IEEE Std 1364-2005 (Verilog 2005).

The following are the steps for evaluating an assignment:
— Determine the size of the right-hand side by the standard assignment size determination rules (see 5.4).
— If needed, extend the size of the right-hand side, performing sign extension if, and only if, the type of the right-hand side is signed.

Actual Behavior

The LHS extension to match RHS is done without considering the sign bit. So it produces essentially:

always @(*) begin
    data_ref_o[offset_i+:4] = 4'b1111;
    data_o[offset_i+:4]     = 4'b0001; // unsigned from signed 1'sb1
end

[daglem]

I don't know whether #4065 helps - if not, I have code to simplify bit slice assignment in simplify.cc, and I can ensure that this simplified code works for the case above and make a PR once #4065 and #3875 are merged 😅

[phsauter]

#4065 does not seem to fix this, it still produces:

assign _01_ = { 29'h00000000, offset_i } + 32'd0;
assign _05_ = $signed({ 1'h0, _01_ }) - $signed(32'd0);
assign _03_ = - $signed(_05_);
assign data_o = $signed(_03_) < 0 ? 1'h1 << - _03_ : 1'h1 >> _03_;

note the last line where it is still 1'h1 (unsigned one bit).

[daglem]

Here is the output which could have been generated if only a few PRs were to be merged 😉

/* Generated by Yosys 0.36+8 (git sha1 74af59a70, gcc 13.2.1 -fPIC -Os) */

module test(offset_i, data_o, data_ref_o);
  wire [31:0] _0_;
  wire [31:0] _1_;
  wire [32:0] _2_;
  wire [32:0] _3_;
  output [15:0] data_o;
  wire [15:0] data_o;
  output [15:0] data_ref_o;
  wire [15:0] data_ref_o;
  input [2:0] offset_i;
  wire [2:0] offset_i;
  assign _1_ = { 29'h00000000, offset_i } + 32'd0;
  assign _0_ = { 29'h00000000, offset_i } + 32'd0;
  assign _2_ = - $signed({ 1'h0, _0_ });
  assign _3_ = - $signed({ 1'h0, _1_ });
  assign data_ref_o = $signed(_2_) < 0 ? 4'hf << - _2_ : 4'hf >> _2_;
  assign data_o = $signed(_3_) < 0 ? 4'hf << - _3_ : 4'hf >> _3_;
endmodule

[phsauter]

Now that does look correct, thanks!

1. Do you have an ETA on the merges?
2. Can you tag this issue once its merged so I now when I can remove the patches that are currently needed

Personally I would consider this to be high-priority since its in the frontend and therefore likely influences absolutely every project (most likely even LEC).

[daglem]

1. I can't do any merging, however unless I'm mistaken it's been agreed that what's in Respect the sign of the right operand of AST_SHIFT and AST_SHIFTX #4065 can be merged right away, and @povik just wants to do an extra check on Optimization of nowrshmsk #3875 before that can be merged.
2. Sure, once the two PRs above are merged, I'll make a third PR which tags this issue as fixed.

[daglem]

@nakengelhardt Can you please merge #4065 and and #4066?

[daglem]

@phsauter I took the liberty to add your test to #4069.

[daglem]

@phsauter FYI if you need sign extension working before all this hopefully gets merged, you can add (* nowrshmsk *) on the variable in question in the mean time.