From ca01de22b6943a358a1f7014b6e1f1d2a8c86298 Mon Sep 17 00:00:00 2001 From: Dag Lem Date: Mon, 13 Mar 2023 21:29:11 +0100 Subject: [PATCH 1/9] Added nordshift attribute nordshift on a net or variable yields generation of muxes instead of shift circuits for dynamic rvalue indexing, akin to nowrshmsk for lvalue indexing. To facilitate this, the AST transformations for rvalue indexing are moved from genrtlil.cc to simplify.cc, bringing them in line with transformations for lvalue indexing. --- README.md | 3 + frontends/ast/genrtlil.cc | 94 ++++++-------------- frontends/ast/simplify.cc | 180 ++++++++++++++++++++++++++++++++++++++ kernel/constids.inc | 1 + 4 files changed, 212 insertions(+), 66 deletions(-) diff --git a/README.md b/README.md index d215d844201..1987f05422a 100644 --- a/README.md +++ b/README.md @@ -313,6 +313,9 @@ Verilog Attributes and non-standard features - The ``nowrshmsk`` attribute on a register prohibits the generation of shift-and-mask type circuits for writing to bit slices of that register. +- The ``nordshift`` attribute on a net or variable prohibits the generation of + shift type circuits for reading bit slices from that data object. + - The ``onehot`` attribute on wires mark them as one-hot state register. This is used for example for memory port sharing and set by the fsm_map pass. diff --git a/frontends/ast/genrtlil.cc b/frontends/ast/genrtlil.cc index 3d47bd3c0f1..5172938e42a 100644 --- a/frontends/ast/genrtlil.cc +++ b/frontends/ast/genrtlil.cc @@ -1512,8 +1512,6 @@ RTLIL::SigSpec AstNode::genRTLIL(int width_hint, bool sign_hint) } // simply return the corresponding RTLIL::SigSpec for an AST_IDENTIFIER node - // for identifiers with dynamic bit ranges (e.g. "foo[bar]" or "foo[bar+3:bar]") a - // shifter cell is created and the output signal of this cell is returned case AST_IDENTIFIER: { RTLIL::Wire *wire = NULL; @@ -1592,8 +1590,8 @@ RTLIL::SigSpec AstNode::genRTLIL(int width_hint, bool sign_hint) use_const_chunk: if (children.size() != 0) { - if (children[0]->type != AST_RANGE) - input_error("Single range expected.\n"); + if (children[0]->type != AST_RANGE || !children[0]->range_valid) + input_error("Single static range expected.\n"); int source_width = id2ast->range_left - id2ast->range_right + 1; int source_offset = id2ast->range_right; int chunk_left = source_width - 1; @@ -1606,70 +1604,34 @@ RTLIL::SigSpec AstNode::genRTLIL(int width_hint, bool sign_hint) chunk_right = chunk.offset; } - if (!children[0]->range_valid) { - AstNode *left_at_zero_ast = children[0]->children[0]->clone_at_zero(); - AstNode *right_at_zero_ast = children[0]->children.size() >= 2 ? children[0]->children[1]->clone_at_zero() : left_at_zero_ast->clone(); - while (left_at_zero_ast->simplify(true, 1, -1, false)) { } - while (right_at_zero_ast->simplify(true, 1, -1, false)) { } - if (left_at_zero_ast->type != AST_CONSTANT || right_at_zero_ast->type != AST_CONSTANT) - input_error("Unsupported expression on dynamic range select on signal `%s'!\n", str.c_str()); - int width = abs(int(left_at_zero_ast->integer - right_at_zero_ast->integer)) + 1; - AstNode *fake_ast = new AstNode(AST_NONE, clone(), children[0]->children.size() >= 2 ? - children[0]->children[1]->clone() : children[0]->children[0]->clone()); - fake_ast->children[0]->delete_children(); - if (member_node) - fake_ast->children[0]->set_attribute(ID::wiretype, member_node->clone()); - - int fake_ast_width = 0; - bool fake_ast_sign = true; - fake_ast->children[1]->detectSignWidth(fake_ast_width, fake_ast_sign); - RTLIL::SigSpec shift_val = fake_ast->children[1]->genRTLIL(fake_ast_width, fake_ast_sign); - - if (source_offset != 0) { - shift_val = current_module->Sub(NEW_ID, shift_val, source_offset, fake_ast_sign); - fake_ast->children[1]->is_signed = true; - } - if (id2ast->range_swapped) { - shift_val = current_module->Sub(NEW_ID, RTLIL::SigSpec(source_width - width), shift_val, fake_ast_sign); - fake_ast->children[1]->is_signed = true; - } - if (GetSize(shift_val) >= 32) - fake_ast->children[1]->is_signed = true; - RTLIL::SigSpec sig = binop2rtlil(fake_ast, ID($shiftx), width, fake_ast->children[0]->genRTLIL(), shift_val); - delete left_at_zero_ast; - delete right_at_zero_ast; - delete fake_ast; - return sig; + chunk.width = children[0]->range_left - children[0]->range_right + 1; + chunk.offset += children[0]->range_right - source_offset; + if (id2ast->range_swapped) + chunk.offset = source_width - (chunk.offset + chunk.width); + if (chunk.offset > chunk_left || chunk.offset + chunk.width < chunk_right) { + if (chunk.width == 1) + log_file_warning(filename, location.first_line, "Range select out of bounds on signal `%s': Setting result bit to undef.\n", + str.c_str()); + else + log_file_warning(filename, location.first_line, "Range select [%d:%d] out of bounds on signal `%s': Setting all %d result bits to undef.\n", + children[0]->range_left, children[0]->range_right, str.c_str(), chunk.width); + chunk = RTLIL::SigChunk(RTLIL::State::Sx, chunk.width); } else { - chunk.width = children[0]->range_left - children[0]->range_right + 1; - chunk.offset += children[0]->range_right - source_offset; - if (id2ast->range_swapped) - chunk.offset = source_width - (chunk.offset + chunk.width); - if (chunk.offset > chunk_left || chunk.offset + chunk.width < chunk_right) { - if (chunk.width == 1) - log_file_warning(filename, location.first_line, "Range select out of bounds on signal `%s': Setting result bit to undef.\n", - str.c_str()); - else - log_file_warning(filename, location.first_line, "Range select [%d:%d] out of bounds on signal `%s': Setting all %d result bits to undef.\n", - children[0]->range_left, children[0]->range_right, str.c_str(), chunk.width); - chunk = RTLIL::SigChunk(RTLIL::State::Sx, chunk.width); - } else { - if (chunk.offset + chunk.width - 1 > chunk_left) { - add_undef_bits_msb = (chunk.offset + chunk.width - 1) - chunk_left; - chunk.width -= add_undef_bits_msb; - } - if (chunk.offset < chunk_right) { - add_undef_bits_lsb = chunk_right - chunk.offset; - chunk.width -= add_undef_bits_lsb; - chunk.offset += add_undef_bits_lsb; - } - if (add_undef_bits_lsb) - log_file_warning(filename, location.first_line, "Range [%d:%d] select out of bounds on signal `%s': Setting %d LSB bits to undef.\n", - children[0]->range_left, children[0]->range_right, str.c_str(), add_undef_bits_lsb); - if (add_undef_bits_msb) - log_file_warning(filename, location.first_line, "Range [%d:%d] select out of bounds on signal `%s': Setting %d MSB bits to undef.\n", - children[0]->range_left, children[0]->range_right, str.c_str(), add_undef_bits_msb); + if (chunk.offset + chunk.width - 1 > chunk_left) { + add_undef_bits_msb = (chunk.offset + chunk.width - 1) - chunk_left; + chunk.width -= add_undef_bits_msb; + } + if (chunk.offset < chunk_right) { + add_undef_bits_lsb = chunk_right - chunk.offset; + chunk.width -= add_undef_bits_lsb; + chunk.offset += add_undef_bits_lsb; } + if (add_undef_bits_lsb) + log_file_warning(filename, location.first_line, "Range [%d:%d] select out of bounds on signal `%s': Setting %d LSB bits to undef.\n", + children[0]->range_left, children[0]->range_right, str.c_str(), add_undef_bits_lsb); + if (add_undef_bits_msb) + log_file_warning(filename, location.first_line, "Range [%d:%d] select out of bounds on signal `%s': Setting %d MSB bits to undef.\n", + children[0]->range_left, children[0]->range_right, str.c_str(), add_undef_bits_msb); } } diff --git a/frontends/ast/simplify.cc b/frontends/ast/simplify.cc index 3d8478ef160..3b17169ed2d 100644 --- a/frontends/ast/simplify.cc +++ b/frontends/ast/simplify.cc @@ -1382,6 +1382,8 @@ bool AstNode::simplify(bool const_fold, int stage, int width_hint, bool sign_hin case AST_ASSIGN: while (!children[0]->basic_prep && children[0]->simplify(false, stage, -1, false) == true) did_something = true; + if (type == AST_ASSIGN && children[0]->type == AST_IDENTIFIER && children[0]->children.size() > 0 && !children[0]->children[0]->range_valid) + log_error("Non-constant range in continuous assignment to %s at %s.\n", children[0]->str.c_str(), loc_string().c_str()); while (!children[1]->basic_prep && children[1]->simplify(false, stage, -1, false) == true) did_something = true; children[0]->detectSignWidth(backup_width_hint, backup_sign_hint); @@ -2310,6 +2312,184 @@ bool AstNode::simplify(bool const_fold, int stage, int width_hint, bool sign_hin goto apply_newNode; } + // Rewrite dynamic indexing of rvalue packed dimensions. + if (type == AST_IDENTIFIER && id2ast != NULL && id2ast->type != AST_MEMORY && !in_lvalue && + GetSize(children) == 1 && children[0]->type == AST_RANGE && !children[0]->range_valid) + { + AST::AstNode *member_node = get_struct_member(this); + int wire_width = member_node ? + member_node->range_left - member_node->range_right + 1 : + id2ast->range_left - id2ast->range_right + 1; + int wire_offset = id2ast->range_right; + + AstNode *range = children[0]; + int result_width; + if (!try_determine_range_width(range, result_width)) + input_error("Unsupported expression on dynamic range select on signal `%s'!\n", str.c_str()); + AstNode *shift_expr = range->children.size() >= 2 ? range->children[1]->clone() : range->children[0]->clone(); + + int shift_expr_width_hint; + bool shift_expr_sign_hint; + shift_expr->detectSignWidth(shift_expr_width_hint, shift_expr_sign_hint); + + bool use_case_method = id2ast->get_bool_attribute(ID::nordshift); + + if (use_case_method) { + // AST_CASE with an AST_COND for each possible bit slice. + + int stride = 1; + long long bitno_div = stride; + int shift_expr_width_max = shift_expr_width_hint; + + if (member_node) { + // Clamp chunk to range of member within struct/union. + log_assert(wire_offset == 0 && !id2ast->range_swapped); + + // When the (* nordshift *) attribute is set, a CASE block is generated below + // to select the indexed bit slice. When a multirange array is indexed, the + // start of each possible slice is separated by the bit stride of the last + // index dimension, and we can optimize the CASE block accordingly. + // The dimension of the original array expression is saved in the 'integer' field. + int dims = integer; + stride = wire_width; + for (int dim = 0; dim < dims; dim++) { + stride /= get_struct_range_width(member_node, dim); + } + bitno_div = stride; + } else { + // Extract (index)*(width) from non_opt_range pattern (@selfsz@((index)*(width)))+(0). + AstNode *lsb_expr = + shift_expr->type == AST_ADD && shift_expr->children[0]->type == AST_SELFSZ && + shift_expr->children[1]->type == AST_CONSTANT && shift_expr->children[1]->integer == 0 ? + shift_expr->children[0]->children[0] : + shift_expr; + + // Extract stride from indexing of two-dimensional packed arrays and + // variable slices on the form src[i*stride +: width]. + if (lsb_expr->type == AST_MUL && + (lsb_expr->children[0]->type == AST_CONSTANT || + lsb_expr->children[1]->type == AST_CONSTANT)) + { + int stride_ix = lsb_expr->children[1]->type == AST_CONSTANT; + stride = (int)lsb_expr->children[stride_ix]->integer; + bitno_div = stride != 0 ? stride : 1; + + // Check whether i*stride can overflow. + int i_width; + bool i_sign; + lsb_expr->children[1 - stride_ix]->detectSignWidth(i_width, i_sign); + int stride_width; + bool stride_sign; + lsb_expr->children[stride_ix]->detectSignWidth(stride_width, stride_sign); + shift_expr_width_max = std::max(i_width, stride_width); + // Stride width calculated from actual stride value. + stride_width = std::ceil(std::log2(std::abs(stride))); + + if (i_width + stride_width > shift_expr_width_max) { + // For (truncated) i*stride to be within the range of dst, the following must hold: + // i*stride ≡ bitno (mod shift_mod), i.e. + // i*stride = k*shift_mod + bitno + // + // The Diophantine equation on the form ax + by = c: + // stride*i - shift_mod*k = bitno + // has solutions iff c is a multiple of d = gcd(a, b), i.e. + // bitno mod gcd(stride, shift_mod) = 0 + // + // long long is at least 64 bits in C++11 + long long shift_mod = 1ll << (shift_expr_width_max - shift_expr_sign_hint); + // std::gcd requires C++17 + // bitno_div = std::gcd(stride, shift_mod); + bitno_div = gcd((long long)stride, shift_mod); + } + } + } + + // long long is at least 64 bits in C++11 + long long max_offset = (1ll << (shift_expr_width_max - shift_expr_sign_hint)) - 1; + long long min_offset = shift_expr_sign_hint ? -(1ll << (shift_expr_width_max - 1)) : 0; + + if (!shift_expr_sign_hint && max_offset >= wire_offset + wire_width) { + // When an unsigned address expression can go beyond the address range, + // we must handle potential overflows causing the lower bits of the wire + // to be selected. + min_offset = -((max_offset + 1) >> 1); + } + + // Temporary register to replace the current rvalue; used as lvalue in AST_COND assignments. + AstNode *lvalue = mktemp_logic("$bitsel$"+str+"$", current_ast_mod, current_block, result_width - 1, 0, false); + + AstNode *assign = new AstNode(AST_CASE, shift_expr); + for (int i = 1 - result_width; i < wire_width; i++) { + // Out of range indexes are handled in genrtlil.cc + int start_bit = wire_offset + i; + int end_bit = start_bit + result_width - 1; + // Check whether the current index can be generated by shift_expr. + if (start_bit < min_offset || start_bit > max_offset) + continue; + if (start_bit%bitno_div != 0 || (stride == 0 && start_bit != 0)) + continue; + AstNode *cond = new AstNode(AST_COND, mkconst_int(start_bit, shift_expr_sign_hint, shift_expr_width_max)); + AstNode *rvalue = clone(); + rvalue->delete_children(); + if (member_node) + rvalue->set_attribute(ID::wiretype, member_node->clone()); + rvalue->children.push_back(new AstNode(AST_RANGE, + node_int(end_bit), node_int(start_bit))); + cond->children.push_back(new AstNode(AST_BLOCK, new AstNode(AST_ASSIGN_EQ, lvalue->clone(), rvalue))); + assign->children.push_back(cond); + } + + // Default to x bits for out of range addresses. + std::vector x_const(result_width, RTLIL::State::Sx); + assign->children.push_back(new AstNode(AST_COND, + new AstNode(AST_DEFAULT), + new AstNode(AST_BLOCK, + new AstNode(AST_ASSIGN_EQ, + lvalue->clone(), AstNode::mkconst_bits(x_const, false))))); + + if (current_block) { + size_t assign_idx = 0; + while (assign_idx < current_block->children.size() && current_block->children[assign_idx] != current_block_child) + assign_idx++; + log_assert(assign_idx < current_block->children.size()); + current_block->children.insert(current_block->children.begin()+assign_idx, assign); + } else { + AstNode *proc = new AstNode(AST_ALWAYS, new AstNode(AST_BLOCK, assign)); + current_ast_mod->children.push_back(proc); + } + + newNode = lvalue; + } else { + // Shift to access the indexed bit slice. + // This is adapted from old code in genrtlil.cc + AstNode *rvalue = clone(); + rvalue->delete_children(); + if (member_node) + rvalue->set_attribute(ID::wiretype, member_node->clone()); + + if (wire_offset != 0) { + // Offset the shift amount by the lower bound of the dimension. + shift_expr = new AstNode(AST_SUB, shift_expr, node_int(wire_offset)); + } + if (id2ast->range_swapped) { + // Reflect the shift amount if the dimension is swapped. + shift_expr = new AstNode(AST_SUB, node_int(wire_width - result_width), shift_expr); + } + + if (!shift_expr_sign_hint && (wire_offset != 0 || id2ast->range_swapped || shift_expr_width_hint >= 32)) { + // Handle potential unsigned overflows, causing the lower bits of the wire to be selected. + shift_expr = new AstNode(AST_TO_SIGNED, shift_expr); + } + + // Shift rvalue to the right so that the bit slice starts at bit 0. + newNode = new AstNode(AST_CAST_SIZE, + node_int(result_width), + new AstNode(AST_SHIFTX, rvalue, shift_expr)); + } + + goto apply_newNode; + } + if (type == AST_WHILE) input_error("While loops are only allowed in constant functions!\n"); diff --git a/kernel/constids.inc b/kernel/constids.inc index 3052afb49e1..bb2a8faad54 100644 --- a/kernel/constids.inc +++ b/kernel/constids.inc @@ -140,6 +140,7 @@ X(nolatches) X(nomem2init) X(nomem2reg) X(nomeminit) +X(nordshift) X(nosync) X(nowrshmsk) X(no_ram) From 8f8a7b9b0f14012f83b2ef4118e6ebfdc541a129 Mon Sep 17 00:00:00 2001 From: Dag Lem Date: Sat, 9 Dec 2023 22:11:35 +0100 Subject: [PATCH 2/9] Add torture test for (* nordshift *) and (* nowrshmsk *) --- tests/simple/partsel_nordshift_nowrshmsk.v | 151 +++++++++++++++++++++ 1 file changed, 151 insertions(+) create mode 100644 tests/simple/partsel_nordshift_nowrshmsk.v diff --git a/tests/simple/partsel_nordshift_nowrshmsk.v b/tests/simple/partsel_nordshift_nowrshmsk.v new file mode 100644 index 00000000000..b1def986b7d --- /dev/null +++ b/tests/simple/partsel_nordshift_nowrshmsk.v @@ -0,0 +1,151 @@ +module partsel_test001(input [2:0] idx, (* nordshift *) input [31:0] data, output [3:0] slice_up, slice_down); +wire [5:0] offset = idx << 2; +assign slice_up = data[offset +: 4]; +assign slice_down = data[offset + 3 -: 4]; +endmodule + +module partsel_test002 ( + input clk, rst, + (* nordshift *) input [7:0] a, + (* nordshift *) input [0:7] b, + (* nordshift *) input [1:0] s, + output [7:0] x1, x2, x3, + output [0:7] x4, x5, x6, + output [7:0] y1, y2, y3, + output [0:7] y4, y5, y6, + output [7:0] z1, z2, z3, + output [0:7] z4, z5, z6, + output [7:0] w1, w2, w3, + output [0:7] w4, w5, w6, + output [7:0] p1, p2, p3, p4, p5, p6, + output [0:7] q1, q2, q3, q4, q5, q6, + output reg [7:0] r1, + output reg [0:7] r2 +); + +assign x1 = a, x2 = a + b, x3 = b; +assign x4 = a, x5 = a + b, x6 = b; +assign y1 = a[4 +: 3], y2 = a[4 +: 3] + b[4 +: 3], y3 = b[4 +: 3]; +assign y4 = a[4 +: 3], y5 = a[4 +: 3] + b[4 +: 3], y6 = b[4 +: 3]; +assign z1 = a[4 -: 3], z2 = a[4 -: 3] + b[4 -: 3], z3 = b[4 -: 3]; +assign z4 = a[4 -: 3], z5 = a[4 -: 3] + b[4 -: 3], z6 = b[4 -: 3]; +assign w1 = a[6:3], w2 = a[6:3] + b[3:6], w3 = b[3:6]; +assign w4 = a[6:3], w5 = a[6:3] + b[3:6], w6 = b[3:6]; +assign p1 = a[s], p2 = b[s], p3 = a[s+2 +: 2], p4 = b[s+2 +: 2], p5 = a[s+2 -: 2], p6 = b[s+2 -: 2]; +assign q1 = a[s], q2 = b[s], q3 = a[s+2 +: 2], q4 = b[s+2 +: 2], q5 = a[s+2 -: 2], q6 = b[s+2 -: 2]; + +always @(posedge clk) begin + if (rst) begin + { r1, r2 } = 16'h1337 ^ {a, b}; + end else begin + case (s) + 0: begin + r1[3:0] <= r2[0:3] ^ x1; + r2[4:7] <= r1[7:4] ^ x4; + end + 1: begin + r1[2 +: 3] <= r2[5 -: 3] + x1; + r2[3 +: 3] <= r1[6 -: 3] + x4; + end + 2: begin + r1[6 -: 3] <= r2[3 +: 3] - x1; + r2[7 -: 3] <= r1[4 +: 3] - x4; + end + 3: begin + r1 <= r2; + r2 <= r1; + end + endcase + end +end + +endmodule + +module partsel_test003(input [2:0] a, b, (* nordshift *) input [31:0] din, output [3:0] dout); +assign dout = din[a*b +: 2]; +endmodule + +module partsel_test004 ( + (* nordshift *) input [31:0] din, + input signed [4:0] n, + (* nowrshmsk *) output reg [31:0] dout +); + always @(*) begin + dout = 0; + dout[n+1 +: 2] = din[n +: 2]; + end +endmodule + + +module partsel_test005 ( + (* nordshift *) input [31:0] din, + input signed [4:0] n, + (* nowrshmsk *) output reg [31:0] dout +); + always @(*) begin + dout = 0; + dout[n+1] = din[n]; + end +endmodule + +module partsel_test006 ( + (* nordshift *) input [31:-32] din, + input signed [4:0] n, + (* nowrshmsk *) output reg [31:-32] dout +); + always @(*) begin + dout = 0; + dout[n+1 +: 2] = din[n +: 2]; + end +endmodule + + +module partsel_test007 ( + (* nordshift *) input [31:-32] din, + input signed [4:0] n, + (* nowrshmsk *) output reg [31:-32] dout +); + always @(*) begin + dout = 0; + dout[n+1] = din[n]; + end +endmodule + + +module partsel_test008 ( + (* nordshift *) input [127:0] din, + input [3:0] idx, + input [4:0] uoffset, + input signed [4:0] soffset, + output [ 7:0] dout0, + output [ 7:0] dout1, + output [ 7:0] dout2, + output [ 7:0] dout3, + output [ 3:0] dout4, + output [ 3:0] dout5, + output [ 3:0] dout6, + output [ 3:0] dout7, + output [ 3:0] dout8, + output [11:0] dout9, + output [11:0] dout10, + output [11:0] dout11 +); + +// common: block-select with offsets +assign dout0 = din[idx*8 +uoffset +:8]; +assign dout1 = din[idx*8 -uoffset +:8]; +assign dout2 = din[idx*8 +soffset +:8]; +assign dout3 = din[idx*8 -soffset +:8]; + +// only partial block used +assign dout4 = din[idx*8 +uoffset +:4]; +assign dout5 = din[idx*8 -uoffset +:4]; +assign dout6 = din[idx*8 +soffset +:4]; +assign dout7 = din[idx*8 -soffset +:4]; + +// uncommon: more than one block used +assign dout8 = din[idx*8 +uoffset +:12]; +assign dout9 = din[idx*8 -uoffset +:12]; +assign dout10 = din[idx*8 +soffset +:12]; +assign dout11 = din[idx*8 -soffset +:12]; +endmodule From 608bc5889a5727ed69c44cb62ed4366a62132d49 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Povi=C5=A1er?= Date: Thu, 22 Feb 2024 16:57:45 +0100 Subject: [PATCH 3/9] ast: Improve obviousness when rewriting dynamic indexing --- frontends/ast/simplify.cc | 33 ++++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/frontends/ast/simplify.cc b/frontends/ast/simplify.cc index 3b17169ed2d..f32334da3f2 100644 --- a/frontends/ast/simplify.cc +++ b/frontends/ast/simplify.cc @@ -26,6 +26,7 @@ * */ +#include "kernel/yosys.h" #include "kernel/log.h" #include "libs/sha1/sha1.h" #include "frontends/verilog/verilog_frontend.h" @@ -2467,18 +2468,28 @@ bool AstNode::simplify(bool const_fold, int stage, int width_hint, bool sign_hin if (member_node) rvalue->set_attribute(ID::wiretype, member_node->clone()); - if (wire_offset != 0) { - // Offset the shift amount by the lower bound of the dimension. - shift_expr = new AstNode(AST_SUB, shift_expr, node_int(wire_offset)); - } - if (id2ast->range_swapped) { - // Reflect the shift amount if the dimension is swapped. - shift_expr = new AstNode(AST_SUB, node_int(wire_width - result_width), shift_expr); - } + // Insert a self-sizing barrier to prevent the rewritten AST from + // influencing the size of the operators in `shift_expr` + shift_expr = new AstNode(AST_SELFSZ, shift_expr); + + // Decode the index based on wire dimensions + int idx_signed_nbits = shift_expr_width_hint + !shift_expr_sign_hint; + if (!id2ast->range_swapped) { + int raw_idx_nbits = 1 + std::max(idx_signed_nbits, ceil_log2(std::abs(wire_offset) + 1) + 1); + shift_expr = new AstNode(AST_SUB, + new AstNode(AST_TO_SIGNED, + new AstNode(AST_CAST_SIZE, node_int(raw_idx_nbits), shift_expr) + ), + node_int(wire_offset)); + } else { + int offset = wire_width - result_width + wire_offset; + int raw_idx_nbits = 1 + std::max(idx_signed_nbits, ceil_log2(std::abs(offset)) + 1); - if (!shift_expr_sign_hint && (wire_offset != 0 || id2ast->range_swapped || shift_expr_width_hint >= 32)) { - // Handle potential unsigned overflows, causing the lower bits of the wire to be selected. - shift_expr = new AstNode(AST_TO_SIGNED, shift_expr); + shift_expr = new AstNode(AST_SUB, node_int(offset), + new AstNode(AST_TO_SIGNED, + new AstNode(AST_CAST_SIZE, node_int(raw_idx_nbits), shift_expr) + ) + ); } // Shift rvalue to the right so that the bit slice starts at bit 0. From 1597dfc192f6d94a7a4f086273831b92e1fc7e61 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Povi=C5=A1er?= Date: Fri, 23 Feb 2024 15:13:46 +0100 Subject: [PATCH 4/9] ast: Catch up with struct member changes --- frontends/ast/simplify.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontends/ast/simplify.cc b/frontends/ast/simplify.cc index f32334da3f2..baaf486e512 100644 --- a/frontends/ast/simplify.cc +++ b/frontends/ast/simplify.cc @@ -2317,7 +2317,7 @@ bool AstNode::simplify(bool const_fold, int stage, int width_hint, bool sign_hin if (type == AST_IDENTIFIER && id2ast != NULL && id2ast->type != AST_MEMORY && !in_lvalue && GetSize(children) == 1 && children[0]->type == AST_RANGE && !children[0]->range_valid) { - AST::AstNode *member_node = get_struct_member(this); + AST::AstNode *member_node = get_struct_member(); int wire_width = member_node ? member_node->range_left - member_node->range_right + 1 : id2ast->range_left - id2ast->range_right + 1; @@ -2354,7 +2354,7 @@ bool AstNode::simplify(bool const_fold, int stage, int width_hint, bool sign_hin int dims = integer; stride = wire_width; for (int dim = 0; dim < dims; dim++) { - stride /= get_struct_range_width(member_node, dim); + stride /= member_node->dimensions[dim].range_width; } bitno_div = stride; } else { From 36257c31bfac49b1385255a3e01de52bd273a4b6 Mon Sep 17 00:00:00 2001 From: Dag Lem Date: Tue, 27 Feb 2024 16:01:19 +0100 Subject: [PATCH 5/9] Make multidimensional packed arrays play nicely with new rvalue indexing --- frontends/ast/simplify.cc | 105 ++++++++++++++++++++------------------ 1 file changed, 54 insertions(+), 51 deletions(-) diff --git a/frontends/ast/simplify.cc b/frontends/ast/simplify.cc index baaf486e512..ffebc13c41c 100644 --- a/frontends/ast/simplify.cc +++ b/frontends/ast/simplify.cc @@ -2067,57 +2067,6 @@ bool AstNode::simplify(bool const_fold, int stage, int width_hint, bool sign_hin } } - // Resolve multidimensional array access. - if (type == AST_IDENTIFIER && !basic_prep && id2ast && (id2ast->type == AST_WIRE || id2ast->type == AST_MEMORY) && - children.size() > 0 && (children[0]->type == AST_RANGE || children[0]->type == AST_MULTIRANGE)) - { - int dims_sel = children[0]->type == AST_MULTIRANGE ? children[0]->children.size() : 1; - // Save original number of dimensions for $size() etc. - integer = dims_sel; - - // Split access into unpacked and packed parts. - AstNode *unpacked_range = nullptr; - AstNode *packed_range = nullptr; - - if (id2ast->unpacked_dimensions) { - if (id2ast->unpacked_dimensions > 1) { - // Flattened range for access to unpacked dimensions. - unpacked_range = make_index_range(id2ast, true); - } else { - // Index into one-dimensional unpacked part; unlink simple range node. - AstNode *&range = children[0]->type == AST_MULTIRANGE ? children[0]->children[0] : children[0]; - unpacked_range = range; - range = nullptr; - } - } - - if (dims_sel > id2ast->unpacked_dimensions) { - if (GetSize(id2ast->dimensions) - id2ast->unpacked_dimensions > 1) { - // Flattened range for access to packed dimensions. - packed_range = make_index_range(id2ast, false); - } else { - // Index into one-dimensional packed part; unlink simple range node. - AstNode *&range = children[0]->type == AST_MULTIRANGE ? children[0]->children[dims_sel - 1] : children[0]; - packed_range = range; - range = nullptr; - } - } - - for (auto &it : children) - delete it; - children.clear(); - - if (unpacked_range) - children.push_back(unpacked_range); - - if (packed_range) - children.push_back(packed_range); - - fixup_hierarchy_flags(); - basic_prep = true; - did_something = true; - } - // trim/extend parameters if (type == AST_PARAMETER || type == AST_LOCALPARAM || type == AST_ENUM_ITEM) { if (children.size() > 1 && children[1]->type == AST_RANGE) { @@ -2256,6 +2205,60 @@ bool AstNode::simplify(bool const_fold, int stage, int width_hint, bool sign_hin } } + // Resolve multidimensional array access. + if (type == AST_IDENTIFIER && !basic_prep && id2ast && (id2ast->type == AST_WIRE || id2ast->type == AST_MEMORY) && + children.size() > 0 && (children[0]->type == AST_RANGE || children[0]->type == AST_MULTIRANGE)) + { + int dims_sel = children[0]->type == AST_MULTIRANGE ? children[0]->children.size() : 1; + // Save original number of dimensions for $size() etc. + integer = dims_sel; + + // Split access into unpacked and packed parts. + AstNode *unpacked_range = nullptr; + AstNode *packed_range = nullptr; + + if (id2ast->unpacked_dimensions) { + if (id2ast->unpacked_dimensions > 1) { + // Flattened range for access to unpacked dimensions. + unpacked_range = make_index_range(id2ast, true); + } else { + // Index into one-dimensional unpacked part; unlink simple range node. + AstNode *&range = children[0]->type == AST_MULTIRANGE ? children[0]->children[0] : children[0]; + unpacked_range = range; + range = nullptr; + } + } + + if (dims_sel > id2ast->unpacked_dimensions) { + if (GetSize(id2ast->dimensions) - id2ast->unpacked_dimensions > 1) { + // Flattened range for access to packed dimensions. + packed_range = make_index_range(id2ast, false); + } else { + // Index into one-dimensional packed part; unlink simple range node. + AstNode *&range = children[0]->type == AST_MULTIRANGE ? children[0]->children[dims_sel - 1] : children[0]; + packed_range = range; + range = nullptr; + } + } + + for (auto &it : children) + delete it; + children.clear(); + + if (unpacked_range) + children.push_back(unpacked_range); + + if (packed_range) + children.push_back(packed_range); + + for (auto child : children) + while (child->simplify(true, 1, -1, false)) { } + + fixup_hierarchy_flags(); + basic_prep = true; + did_something = true; + } + // split memory access with bit select to individual statements if (type == AST_IDENTIFIER && children.size() == 2 && children[0]->type == AST_RANGE && children[1]->type == AST_RANGE && !in_lvalue && stage == 2) { From 7b2c29955a300c0d1d591681875237cee86ddd16 Mon Sep 17 00:00:00 2001 From: Dag Lem Date: Fri, 1 Mar 2024 18:28:16 +0100 Subject: [PATCH 6/9] Correct self-determined signedness for right operand of AST_SHIFT and AST_SHIFTX AST_CAST_SIZE on the right operand caused an unsigned operand to be signed. This is corrected by handling the right operand like in AST_POW. --- frontends/ast/genrtlil.cc | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/frontends/ast/genrtlil.cc b/frontends/ast/genrtlil.cc index 5172938e42a..a4aa9a11d24 100644 --- a/frontends/ast/genrtlil.cc +++ b/frontends/ast/genrtlil.cc @@ -1762,11 +1762,17 @@ RTLIL::SigSpec AstNode::genRTLIL(int width_hint, bool sign_hint) if (0) { case AST_SHIFTX: type_name = ID($shiftx); } if (0) { case AST_SHIFT: type_name = ID($shift); } { + int right_width; + bool right_signed; + children[1]->detectSignWidth(right_width, right_signed); + // for ordinary shift operators, the right operand is always treated as an unsigned number, + // while the special cells $shift and $shiftx can shift both to the left (negative) and right (positive) + if (type != AST_SHIFT && type != AST_SHIFTX) + right_signed = false; if (width_hint < 0) detectSignWidth(width_hint, sign_hint); RTLIL::SigSpec left = children[0]->genRTLIL(width_hint, sign_hint); - // for $shift and $shiftx, the second operand can be negative - RTLIL::SigSpec right = children[1]->genRTLIL(-1, type == AST_SHIFT || type == AST_SHIFTX); + RTLIL::SigSpec right = children[1]->genRTLIL(right_width, right_signed); int width = width_hint > 0 ? width_hint : left.size(); is_signed = children[0]->is_signed; return binop2rtlil(this, type_name, width, left, right); From c67421e57c8917db0f129850b45db7b0770dcb3a Mon Sep 17 00:00:00 2001 From: Dag Lem Date: Wed, 6 Mar 2024 18:28:10 +0100 Subject: [PATCH 7/9] Add function for minimum number of bits required to store integer values --- kernel/yosys.cc | 33 +++++++++++++++++++++++++++++++++ kernel/yosys_common.h | 1 + 2 files changed, 34 insertions(+) diff --git a/kernel/yosys.cc b/kernel/yosys.cc index efdc54b796c..048c8b74354 100644 --- a/kernel/yosys.cc +++ b/kernel/yosys.cc @@ -73,6 +73,10 @@ #include #include +#if __cplusplus >= 202002L +# include +#endif + #include "libs/json11/json11.hpp" YOSYS_NAMESPACE_BEGIN @@ -159,6 +163,35 @@ int ceil_log2(int x) #endif } +// Calculate the minimum number of bits required to store an integer value. +// Note that min_bit_width(0) == 0. +int min_bit_width(int x, bool is_signed) +{ + int width = 0; + if (is_signed) { + if (x < 0) { + // Compute two's complement of negative value. + // Negative numbers are represented in two's complement form + // in SystemVerilog, and this is also guaranteed by C++20. + // C++20 also makes integral conversion of unsigned integers + // to signed integers well defined. + x = -(unsigned)x; + } + // Add a bit for sign, except for 0 and the most negative number, whose + // two's complement is equal to itself, i.e. the sign bit is still set. + width = (x > 0); + } +#if __cplusplus >= 202002L + return width + std::bit_width((unsigned)x); +#else + while (x) { + width += 1; + x = (unsigned)x >> 1; + } + return width; +#endif +} + int readsome(std::istream &f, char *s, int n) { int rc = int(f.readsome(s, n)); diff --git a/kernel/yosys_common.h b/kernel/yosys_common.h index ec1b5aba2ba..96376793952 100644 --- a/kernel/yosys_common.h +++ b/kernel/yosys_common.h @@ -266,6 +266,7 @@ inline void memhasher() { if (memhasher_active) memhasher_do(); } void yosys_banner(); int ceil_log2(int x) YS_ATTRIBUTE(const); +int min_bit_width(int x, bool is_signed); inline std::string vstringf(const char *fmt, va_list ap) { From 5a63653d9e015617adb106532b868f9543ace2eb Mon Sep 17 00:00:00 2001 From: Dag Lem Date: Wed, 6 Mar 2024 20:09:43 +0100 Subject: [PATCH 8/9] Further simplify rewriting of dynamic indexing This also corrects the calculation of bit widths, using the new function min_bit_width. --- frontends/ast/simplify.cc | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/frontends/ast/simplify.cc b/frontends/ast/simplify.cc index ffebc13c41c..58ffc5a5ac6 100644 --- a/frontends/ast/simplify.cc +++ b/frontends/ast/simplify.cc @@ -2465,7 +2465,6 @@ bool AstNode::simplify(bool const_fold, int stage, int width_hint, bool sign_hin newNode = lvalue; } else { // Shift to access the indexed bit slice. - // This is adapted from old code in genrtlil.cc AstNode *rvalue = clone(); rvalue->delete_children(); if (member_node) @@ -2477,22 +2476,17 @@ bool AstNode::simplify(bool const_fold, int stage, int width_hint, bool sign_hin // Decode the index based on wire dimensions int idx_signed_nbits = shift_expr_width_hint + !shift_expr_sign_hint; + int offset = !id2ast->range_swapped ? wire_offset : wire_width - result_width + wire_offset; + int offset_signed_nbits = min_bit_width(offset, true); + int raw_idx_nbits = 1 + std::max(idx_signed_nbits, offset_signed_nbits); + AstNode *node_offset = mkconst_int(offset, true, offset_signed_nbits); + shift_expr = new AstNode(AST_TO_SIGNED, + new AstNode(AST_CAST_SIZE, node_int(raw_idx_nbits), shift_expr) + ); if (!id2ast->range_swapped) { - int raw_idx_nbits = 1 + std::max(idx_signed_nbits, ceil_log2(std::abs(wire_offset) + 1) + 1); - shift_expr = new AstNode(AST_SUB, - new AstNode(AST_TO_SIGNED, - new AstNode(AST_CAST_SIZE, node_int(raw_idx_nbits), shift_expr) - ), - node_int(wire_offset)); + shift_expr = new AstNode(AST_SUB, shift_expr, node_offset); } else { - int offset = wire_width - result_width + wire_offset; - int raw_idx_nbits = 1 + std::max(idx_signed_nbits, ceil_log2(std::abs(offset)) + 1); - - shift_expr = new AstNode(AST_SUB, node_int(offset), - new AstNode(AST_TO_SIGNED, - new AstNode(AST_CAST_SIZE, node_int(raw_idx_nbits), shift_expr) - ) - ); + shift_expr = new AstNode(AST_SUB, node_offset, shift_expr); } // Shift rvalue to the right so that the bit slice starts at bit 0. From d6dd61d07c95bca6ead1feb649f2c06aea5a950c Mon Sep 17 00:00:00 2001 From: Dag Lem Date: Sat, 9 Mar 2024 12:08:46 +0100 Subject: [PATCH 9/9] Replace calls to ceil(log2(x)) with ceil_log2(x) --- frontends/ast/simplify.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontends/ast/simplify.cc b/frontends/ast/simplify.cc index 58ffc5a5ac6..b5ba9c5d042 100644 --- a/frontends/ast/simplify.cc +++ b/frontends/ast/simplify.cc @@ -2387,7 +2387,7 @@ bool AstNode::simplify(bool const_fold, int stage, int width_hint, bool sign_hin lsb_expr->children[stride_ix]->detectSignWidth(stride_width, stride_sign); shift_expr_width_max = std::max(i_width, stride_width); // Stride width calculated from actual stride value. - stride_width = std::ceil(std::log2(std::abs(stride))); + stride_width = ceil_log2(std::abs(stride)); if (i_width + stride_width > shift_expr_width_max) { // For (truncated) i*stride to be within the range of dst, the following must hold: @@ -3066,7 +3066,7 @@ bool AstNode::simplify(bool const_fold, int stage, int width_hint, bool sign_hin lsb_expr->children[stride_ix]->detectSignWidth(stride_width, stride_sign); max_width = std::max(i_width, stride_width); // Stride width calculated from actual stride value. - stride_width = std::ceil(std::log2(std::abs(stride))); + stride_width = ceil_log2(std::abs(stride)); if (i_width + stride_width > max_width) { // For (truncated) i*stride to be within the range of dst, the following must hold: