From 23cd23efc56147c5f2a8645dccd65ae7c189b89d Mon Sep 17 00:00:00 2001 From: Dag Lem Date: Tue, 12 Dec 2023 13:37:34 +0100 Subject: [PATCH 1/2] Simplify and correct AST for array slice assignment Corrects sign extension of the right hand side, and hopefully makes the code simpler to understand. Fixes #4064 --- frontends/ast/simplify.cc | 111 ++++++++++++++------------------------ 1 file changed, 41 insertions(+), 70 deletions(-) diff --git a/frontends/ast/simplify.cc b/frontends/ast/simplify.cc index 945f286a1f6..51ed9562163 100644 --- a/frontends/ast/simplify.cc +++ b/frontends/ast/simplify.cc @@ -2966,96 +2966,67 @@ bool AstNode::simplify(bool const_fold, int stage, int width_hint, bool sign_hin } } else { // mask and shift operations - - AstNode *wire_mask = new AstNode(AST_WIRE, new AstNode(AST_RANGE, mkconst_int(wire_width-1, true), mkconst_int(0, true))); - wire_mask->str = stringf("$bitselwrite$mask$%s:%d$%d", RTLIL::encode_filename(filename).c_str(), location.first_line, autoidx++); - wire_mask->set_attribute(ID::nosync, AstNode::mkconst_int(1, false)); - wire_mask->is_logic = true; - while (wire_mask->simplify(true, 1, -1, false)) { } - current_ast_mod->children.push_back(wire_mask); - - AstNode *wire_data = new AstNode(AST_WIRE, new AstNode(AST_RANGE, mkconst_int(wire_width-1, true), mkconst_int(0, true))); - wire_data->str = stringf("$bitselwrite$data$%s:%d$%d", RTLIL::encode_filename(filename).c_str(), location.first_line, autoidx++); - wire_data->set_attribute(ID::nosync, AstNode::mkconst_int(1, false)); - wire_data->is_logic = true; - while (wire_data->simplify(true, 1, -1, false)) { } - current_ast_mod->children.push_back(wire_data); - - int shamt_width_hint = -1; - bool shamt_sign_hint = true; - shift_expr->detectSignWidth(shamt_width_hint, shamt_sign_hint); - - AstNode *wire_sel = new AstNode(AST_WIRE, new AstNode(AST_RANGE, mkconst_int(shamt_width_hint-1, true), mkconst_int(0, true))); - wire_sel->str = stringf("$bitselwrite$sel$%s:%d$%d", RTLIL::encode_filename(filename).c_str(), location.first_line, autoidx++); - wire_sel->set_attribute(ID::nosync, AstNode::mkconst_int(1, false)); - wire_sel->is_logic = true; - wire_sel->is_signed = shamt_sign_hint; - while (wire_sel->simplify(true, 1, -1, false)) { } - current_ast_mod->children.push_back(wire_sel); - - did_something = true; - newNode = new AstNode(AST_BLOCK); + // dst = (dst & ~(width'1 << lsb)) | unsigned'(width'(src)) << lsb) AstNode *lvalue = children[0]->clone(); lvalue->delete_children(); if (member_node) lvalue->set_attribute(ID::wiretype, member_node->clone()); - AstNode *ref_mask = new AstNode(AST_IDENTIFIER); - ref_mask->str = wire_mask->str; - ref_mask->id2ast = wire_mask; - ref_mask->was_checked = true; - - AstNode *ref_data = new AstNode(AST_IDENTIFIER); - ref_data->str = wire_data->str; - ref_data->id2ast = wire_data; - ref_data->was_checked = true; - - AstNode *ref_sel = new AstNode(AST_IDENTIFIER); - ref_sel->str = wire_sel->str; - ref_sel->id2ast = wire_sel; - ref_sel->was_checked = true; - AstNode *old_data = lvalue->clone(); if (type == AST_ASSIGN_LE) old_data->lookahead = true; - AstNode *s = new AstNode(AST_ASSIGN_EQ, ref_sel->clone(), shift_expr); - newNode->children.push_back(s); + int shift_width_hint; + bool shift_sign_hint; + shift_expr->detectSignWidth(shift_width_hint, shift_sign_hint); + + // All operations are carried out in a new block. + newNode = new AstNode(AST_BLOCK); + + // Temporary register holding the result of the bit- or part-select position expression. + AstNode *pos = mktemp_logic("$bitselwrite$pos$", current_ast_mod, true, shift_width_hint - 1, 0, shift_sign_hint); + newNode->children.push_back(new AstNode(AST_ASSIGN_EQ, pos, shift_expr)); - AstNode *shamt = ref_sel; + // Calculate lsb from position. + AstNode *shift_val = pos->clone(); - // convert to signed while preserving the sign and value - shamt = new AstNode(AST_CAST_SIZE, mkconst_int(shamt_width_hint + 1, true), shamt); - shamt = new AstNode(AST_TO_SIGNED, shamt); + // If the expression is signed, we must add an extra bit for possible negation of the most negative number. + // If the expression is unsigned, we must add an extra bit for sign. + shift_val = new AstNode(AST_CAST_SIZE, mkconst_int(shift_width_hint + 1, true), shift_val); + if (!shift_sign_hint) + shift_val = new AstNode(AST_TO_SIGNED, shift_val); // offset the shift amount by the lower bound of the dimension - int start_bit = wire_offset; - shamt = new AstNode(AST_SUB, shamt, mkconst_int(start_bit, true)); + if (wire_offset != 0) + shift_val = new AstNode(AST_SUB, shift_val, mkconst_int(wire_offset, true)); // reflect the shift amount if the dimension is swapped if (children[0]->id2ast->range_swapped) - shamt = new AstNode(AST_SUB, mkconst_int(wire_width - result_width, true), shamt); + shift_val = new AstNode(AST_SUB, mkconst_int(wire_width - result_width, true), shift_val); // AST_SHIFT uses negative amounts for shifting left - shamt = new AstNode(AST_NEG, shamt); + shift_val = new AstNode(AST_NEG, shift_val); - AstNode *t; - - t = mkconst_bits(std::vector(result_width, State::S1), false); - t = new AstNode(AST_SHIFT, t, shamt->clone()); - t = new AstNode(AST_ASSIGN_EQ, ref_mask->clone(), t); - newNode->children.push_back(t); - - t = new AstNode(AST_BIT_AND, mkconst_bits(std::vector(result_width, State::S1), false), children[1]->clone()); - t = new AstNode(AST_SHIFT, t, shamt); - t = new AstNode(AST_ASSIGN_EQ, ref_data->clone(), t); - newNode->children.push_back(t); - - t = new AstNode(AST_BIT_AND, old_data, new AstNode(AST_BIT_NOT, ref_mask)); - t = new AstNode(AST_BIT_OR, t, ref_data); - t = new AstNode(type, lvalue, t); - newNode->children.push_back(t); + // dst = (dst & ~(width'1 << lsb)) | unsigned'(width'(src)) << lsb) + did_something = true; + AstNode *bitmask = mkconst_bits(std::vector(result_width, State::S1), false); + newNode->children.push_back( + new AstNode(type, + lvalue, + new AstNode(AST_BIT_OR, + new AstNode(AST_BIT_AND, + old_data, + new AstNode(AST_BIT_NOT, + new AstNode(AST_SHIFT, + bitmask, + shift_val->clone()))), + new AstNode(AST_SHIFT, + new AstNode(AST_TO_UNSIGNED, + new AstNode(AST_CAST_SIZE, + mkconst_int(result_width, true), + children[1]->clone())), + shift_val)))); newNode->fixup_hierarchy_flags(true); } From e0566eafdbdbe0b99b944edb73fe351149dc380a Mon Sep 17 00:00:00 2001 From: Dag Lem Date: Tue, 12 Dec 2023 14:16:45 +0100 Subject: [PATCH 2/2] Add test for rhs sign extension in array slice assignment --- tests/simple/sign_part_assign.v | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 tests/simple/sign_part_assign.v diff --git a/tests/simple/sign_part_assign.v b/tests/simple/sign_part_assign.v new file mode 100644 index 00000000000..5f65ac28402 --- /dev/null +++ b/tests/simple/sign_part_assign.v @@ -0,0 +1,20 @@ +module test ( + offset_i, + data_o, + data_ref_o +); +input wire [ 2:0] offset_i; +output reg [15:0] data_o; +output reg [15:0] data_ref_o; + +always @(*) begin + // defaults + data_o = '0; + data_ref_o = '0; + + // partial assigns + data_ref_o[offset_i+:4] = 4'b1111; // unsigned + data_o[offset_i+:4] = 1'sb1; // sign extension to 4'b1111 +end + +endmodule