From 131e64c53c7f9783ee2ac2917de83bf0b19459c6 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Wed, 27 Dec 2023 22:15:39 -0800 Subject: [PATCH 1/2] Detect reversed part select on inner dimensions The order of the indices of a part select need to match the order in which the dimension of a packed array has been declared. E.g. if the msb is less than the lsb in the declaration it also has to be for the part select. If the order of the part select is the opposite of the declaration this is an error. This works as expected for part selects on the most outer dimensions. But for inner dimensions the current implementation just swaps the msb and lsb of the part select if they are in the wrong order. Refactor this so that an error is reported for both the outer and inner dimensions. Signed-off-by: Lars-Peter Clausen --- elab_expr.cc | 22 ++++------------ elab_lval.cc | 40 ++++++++++++----------------- elab_net.cc | 71 ++++++++++++++++++++++------------------------------ 3 files changed, 51 insertions(+), 82 deletions(-) diff --git a/elab_expr.cc b/elab_expr.cc index 9f1074221..38b401524 100644 --- a/elab_expr.cc +++ b/elab_expr.cc @@ -5843,11 +5843,6 @@ NetExpr* PEIdent::elaborate_expr_net_part_(Design*des, NetScope*scope, if (!flag) return 0; - /* The indices of part selects are signed integers, so allow - negative values. However, the width that they represent is - unsigned. Remember that any order is possible, - i.e., [1:0], [-4:6], etc. */ - unsigned long wid = 1 + labs(msv-lsv); /* But wait... if the part select expressions are not fully defined, then fall back on the tested width. */ if (!parts_defined_flag) { @@ -5878,11 +5873,10 @@ NetExpr* PEIdent::elaborate_expr_net_part_(Design*des, NetScope*scope, // Make this work by finding the indexed slices and // creating a generated slice that spans the whole // range. - long loff, moff; unsigned long lwid, mwid; bool lrc, mrc; - lrc = net->sig()->sb_to_slice(prefix_indices, lsv, loff, lwid); - mrc = net->sig()->sb_to_slice(prefix_indices, msv, moff, mwid); + lrc = net->sig()->sb_to_slice(prefix_indices, lsv, sb_lsb, lwid); + mrc = net->sig()->sb_to_slice(prefix_indices, msv, sb_msb, mwid); if (!mrc || !lrc) { cerr << get_fileline() << ": error: "; cerr << "Part-select [" << msv << ":" << lsv; @@ -5894,15 +5888,7 @@ NetExpr* PEIdent::elaborate_expr_net_part_(Design*des, NetScope*scope, return 0; } ivl_assert(*this, lwid == mwid); - - if (moff > loff) { - sb_lsb = loff; - sb_msb = moff + mwid - 1; - } else { - sb_lsb = moff; - sb_msb = loff + lwid - 1; - } - wid = sb_msb - sb_lsb + 1; + sb_msb += mwid - 1; } else { // This case, the prefix indices are enough to index // down to a single bit/slice. @@ -5951,6 +5937,8 @@ NetExpr* PEIdent::elaborate_expr_net_part_(Design*des, NetScope*scope, } } + unsigned long wid = sb_msb - sb_lsb + 1; + // If the part select covers exactly the entire // vector, then do not bother with it. Return the // signal itself, casting to unsigned if necessary. diff --git a/elab_lval.cc b/elab_lval.cc index 77b7829bb..6b2927602 100644 --- a/elab_lval.cc +++ b/elab_lval.cc @@ -752,15 +752,14 @@ bool PEIdent::elaborate_lval_net_part_(Design*des, const netranges_t&packed = reg->packed_dims(); long loff, moff; - long wid; if (prefix_indices.size()+1 < packed.size()) { // If there are fewer indices then there are packed // dimensions, then this is a range of slices. Calculate // it into a big slice. bool lrc, mrc; - unsigned long tmp_lwid, tmp_mwid; - lrc = reg->sb_to_slice(prefix_indices, lsb, loff, tmp_lwid); - mrc = reg->sb_to_slice(prefix_indices, msb, moff, tmp_mwid); + unsigned long lwid, mwid; + lrc = reg->sb_to_slice(prefix_indices, lsb, loff, lwid); + mrc = reg->sb_to_slice(prefix_indices, msb, moff, mwid); if (!mrc || !lrc) { cerr << get_fileline() << ": error: "; cerr << "Part-select [" << msb << ":" << lsb; @@ -771,33 +770,26 @@ bool PEIdent::elaborate_lval_net_part_(Design*des, des->errors += 1; return 0; } - - if (loff < moff) { - moff = moff + tmp_mwid - 1; - } else { - long ltmp = moff; - moff = loff + tmp_lwid - 1; - loff = ltmp; - } - wid = moff - loff + 1; - + assert(lwid == mwid); + moff += mwid - 1; } else { loff = reg->sb_to_idx(prefix_indices,lsb); moff = reg->sb_to_idx(prefix_indices,msb); - wid = moff - loff + 1; - - if (moff < loff) { - cerr << get_fileline() << ": error: part select " - << reg->name() << "[" << msb<<":"<errors += 1; - return false; - } } + if (moff < loff) { + cerr << get_fileline() << ": error: part select " + << reg->name() << "[" << msb<<":"<errors += 1; + return false; + } + + unsigned long wid = moff - loff + 1; + // Special case: The range winds up selecting the entire // vector. Treat this as no part select at all. - if (loff == 0 && moff == (long)(reg->vector_width()-1)) { + if (loff == 0 && wid == reg->vector_width()) { return true; } diff --git a/elab_net.cc b/elab_net.cc index 9635bf0e8..4d6d8dd86 100644 --- a/elab_net.cc +++ b/elab_net.cc @@ -380,11 +380,10 @@ bool PEIdent::eval_part_select_(Design*des, NetScope*scope, NetNet*sig, // Make this work by finding the indexed slices and // creating a generated slice that spans the whole // range. - long loff, moff; unsigned long lwid, mwid; bool lrc, mrc; - lrc = sig->sb_to_slice(prefix_indices, lsb, loff, lwid); - mrc = sig->sb_to_slice(prefix_indices, msb, moff, mwid); + lrc = sig->sb_to_slice(prefix_indices, lsb, lidx, lwid); + mrc = sig->sb_to_slice(prefix_indices, msb, midx, mwid); if (!mrc || !lrc) { cerr << get_fileline() << ": error: "; cerr << "Part-select [" << msb << ":" << lsb; @@ -396,49 +395,39 @@ bool PEIdent::eval_part_select_(Design*des, NetScope*scope, NetNet*sig, return 0; } ivl_assert(*this, lwid == mwid); - - if (moff > loff) { - lidx = loff; - midx = moff + mwid - 1; - } else { - lidx = moff; - midx = loff + lwid - 1; - } + midx += mwid - 1; } else { - long lidx_tmp = sig->sb_to_idx(prefix_indices, lsb); - long midx_tmp = sig->sb_to_idx(prefix_indices, msb); + lidx = sig->sb_to_idx(prefix_indices, lsb); + midx = sig->sb_to_idx(prefix_indices, msb); + } - /* Detect reversed indices of a part select. */ - if (lidx_tmp > midx_tmp) { - cerr << get_fileline() << ": error: Part select " - << sig->name() << "[" << msb << ":" - << lsb << "] indices reversed." << endl; - cerr << get_fileline() << ": : Did you mean " - << sig->name() << "[" << lsb << ":" - << msb << "]?" << endl; - long tmp = midx_tmp; - midx_tmp = lidx_tmp; - lidx_tmp = tmp; - des->errors += 1; - } + /* Detect reversed indices of a part select. */ + if (lidx > midx) { + cerr << get_fileline() << ": error: Part select " + << sig->name() << "[" << msb << ":" + << lsb << "] indices reversed." << endl; + cerr << get_fileline() << ": : Did you mean " + << sig->name() << "[" << lsb << ":" + << msb << "]?" << endl; + des->errors += 1; - /* Warn about a part select that is out of range. */ - if (midx_tmp >= (long)sig->vector_width() || lidx_tmp < 0) { - cerr << get_fileline() << ": warning: Part select " - << sig->name(); - if (sig->unpacked_dimensions() > 0) { - cerr << "[]"; - } - cerr << "[" << msb << ":" << lsb - << "] is out of range." << endl; + std::swap(lidx, midx); } - /* This is completely out side the signal so just skip it. */ - if (lidx_tmp >= (long)sig->vector_width() || midx_tmp < 0) { - return false; - } - midx = midx_tmp; - lidx = lidx_tmp; + /* Warn about a part select that is out of range. */ + if (midx >= (long)sig->vector_width() || lidx < 0) { + cerr << get_fileline() << ": warning: Part select " + << sig->name(); + if (sig->unpacked_dimensions() > 0) { + cerr << "[]"; + } + cerr << "[" << msb << ":" << lsb + << "] is out of range." << endl; + } + + /* This is completely out side the signal so just skip it. */ + if (lidx >= (long)sig->vector_width() || midx < 0) { + return false; } break; } From 57f8084d0c94806e4158bd17d6231d91fe64795b Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Fri, 29 Dec 2023 16:32:47 -0800 Subject: [PATCH 2/2] Add regression tests for reversed part select indices Check that reversed part selects result in an error. Check this for both right-hand and left-hand side expressions as well as for inner and outer dimensions. Signed-off-by: Lars-Peter Clausen --- ivtest/ivltests/partsel_reversed_idx1.v | 14 ++++++++++++++ ivtest/ivltests/partsel_reversed_idx2.v | 14 ++++++++++++++ ivtest/ivltests/partsel_reversed_idx3.v | 14 ++++++++++++++ ivtest/ivltests/partsel_reversed_idx4.v | 14 ++++++++++++++ ivtest/ivltests/partsel_reversed_idx5.v | 14 ++++++++++++++ ivtest/ivltests/partsel_reversed_idx6.v | 15 +++++++++++++++ ivtest/regress-vvp.list | 6 ++++++ ivtest/vvp_tests/partsel_reversed_idx1.json | 4 ++++ ivtest/vvp_tests/partsel_reversed_idx2.json | 4 ++++ ivtest/vvp_tests/partsel_reversed_idx3.json | 4 ++++ ivtest/vvp_tests/partsel_reversed_idx4.json | 4 ++++ ivtest/vvp_tests/partsel_reversed_idx5.json | 4 ++++ ivtest/vvp_tests/partsel_reversed_idx6.json | 4 ++++ 13 files changed, 115 insertions(+) create mode 100644 ivtest/ivltests/partsel_reversed_idx1.v create mode 100644 ivtest/ivltests/partsel_reversed_idx2.v create mode 100644 ivtest/ivltests/partsel_reversed_idx3.v create mode 100644 ivtest/ivltests/partsel_reversed_idx4.v create mode 100644 ivtest/ivltests/partsel_reversed_idx5.v create mode 100644 ivtest/ivltests/partsel_reversed_idx6.v create mode 100644 ivtest/vvp_tests/partsel_reversed_idx1.json create mode 100644 ivtest/vvp_tests/partsel_reversed_idx2.json create mode 100644 ivtest/vvp_tests/partsel_reversed_idx3.json create mode 100644 ivtest/vvp_tests/partsel_reversed_idx4.json create mode 100644 ivtest/vvp_tests/partsel_reversed_idx5.json create mode 100644 ivtest/vvp_tests/partsel_reversed_idx6.json diff --git a/ivtest/ivltests/partsel_reversed_idx1.v b/ivtest/ivltests/partsel_reversed_idx1.v new file mode 100644 index 000000000..16551be95 --- /dev/null +++ b/ivtest/ivltests/partsel_reversed_idx1.v @@ -0,0 +1,14 @@ +// Check that an inverted part select in a continuous assign is reported as an +// error. + +module test; + + reg [1:0] x; + + assign x[0:1] = 2'b00; // Error: Part select indices swapped + + initial begin + $display("FAILED"); + end + +endmodule diff --git a/ivtest/ivltests/partsel_reversed_idx2.v b/ivtest/ivltests/partsel_reversed_idx2.v new file mode 100644 index 000000000..459732af0 --- /dev/null +++ b/ivtest/ivltests/partsel_reversed_idx2.v @@ -0,0 +1,14 @@ +// Check that an inverted part select on an inner dimension in a continuous +// assign is reported as an error. + +module test; + + reg [1:0][1:0] x; + + assign x[0:1] = 2'b00; // Error: Part select indices swapped + + initial begin + $display("FAILED"); + end + +endmodule diff --git a/ivtest/ivltests/partsel_reversed_idx3.v b/ivtest/ivltests/partsel_reversed_idx3.v new file mode 100644 index 000000000..f8d0ea003 --- /dev/null +++ b/ivtest/ivltests/partsel_reversed_idx3.v @@ -0,0 +1,14 @@ +// Check that an inverted part select in a procedural assign is reported as an +// error. + +module test; + + reg [1:0] x; + + initial begin + x[0:1] = 2'b00; // Error: Part select indices swapped + + $display("FAILED"); + end + +endmodule diff --git a/ivtest/ivltests/partsel_reversed_idx4.v b/ivtest/ivltests/partsel_reversed_idx4.v new file mode 100644 index 000000000..c09eaba3f --- /dev/null +++ b/ivtest/ivltests/partsel_reversed_idx4.v @@ -0,0 +1,14 @@ +// Check that an inverted part select on an inner dimension in a procedural +// assign is reported as an error. + +module test; + + reg [1:0][1:0] x; + + initial begin + x[0:1] = 2'b00; // Error: Part select indices swapped + + $display("FAILED"); + end + +endmodule diff --git a/ivtest/ivltests/partsel_reversed_idx5.v b/ivtest/ivltests/partsel_reversed_idx5.v new file mode 100644 index 000000000..b3daa49c9 --- /dev/null +++ b/ivtest/ivltests/partsel_reversed_idx5.v @@ -0,0 +1,14 @@ +// Check that an inverted part select in an expression is reported as an error. + +module test; + + reg [1:0] x; + reg [1:0] y; + + initial begin + y = x[0:1]; // Error: Part select indices swapped + + $display("FAILED"); + end + +endmodule diff --git a/ivtest/ivltests/partsel_reversed_idx6.v b/ivtest/ivltests/partsel_reversed_idx6.v new file mode 100644 index 000000000..55f92a942 --- /dev/null +++ b/ivtest/ivltests/partsel_reversed_idx6.v @@ -0,0 +1,15 @@ +// Check that an inverted part select on an inner dimension in an expression is +// reported as an error. + +module test; + + reg [1:0][1:0] x; + reg [1:0] y; + + initial begin + y = x[0:1]; // Error: Part select indices swapped + + $display("FAILED"); + end + +endmodule diff --git a/ivtest/regress-vvp.list b/ivtest/regress-vvp.list index 81a6d9fec..20b686c6d 100644 --- a/ivtest/regress-vvp.list +++ b/ivtest/regress-vvp.list @@ -94,6 +94,12 @@ partsel_invalid_idx3 vvp_tests/partsel_invalid_idx3.json partsel_invalid_idx4 vvp_tests/partsel_invalid_idx4.json partsel_invalid_idx5 vvp_tests/partsel_invalid_idx5.json partsel_invalid_idx6 vvp_tests/partsel_invalid_idx6.json +partsel_reversed_idx1 vvp_tests/partsel_reversed_idx1.json +partsel_reversed_idx2 vvp_tests/partsel_reversed_idx2.json +partsel_reversed_idx3 vvp_tests/partsel_reversed_idx3.json +partsel_reversed_idx4 vvp_tests/partsel_reversed_idx4.json +partsel_reversed_idx5 vvp_tests/partsel_reversed_idx5.json +partsel_reversed_idx6 vvp_tests/partsel_reversed_idx6.json param_test3 vvp_tests/param_test3.json param-width vvp_tests/param-width.json param-width-vlog95 vvp_tests/param-width-vlog95.json diff --git a/ivtest/vvp_tests/partsel_reversed_idx1.json b/ivtest/vvp_tests/partsel_reversed_idx1.json new file mode 100644 index 000000000..e251399c1 --- /dev/null +++ b/ivtest/vvp_tests/partsel_reversed_idx1.json @@ -0,0 +1,4 @@ +{ + "type" : "CE", + "source" : "partsel_reversed_idx1.v" +} diff --git a/ivtest/vvp_tests/partsel_reversed_idx2.json b/ivtest/vvp_tests/partsel_reversed_idx2.json new file mode 100644 index 000000000..5d45f578a --- /dev/null +++ b/ivtest/vvp_tests/partsel_reversed_idx2.json @@ -0,0 +1,4 @@ +{ + "type" : "CE", + "source" : "partsel_reversed_idx2.v" +} diff --git a/ivtest/vvp_tests/partsel_reversed_idx3.json b/ivtest/vvp_tests/partsel_reversed_idx3.json new file mode 100644 index 000000000..60522804a --- /dev/null +++ b/ivtest/vvp_tests/partsel_reversed_idx3.json @@ -0,0 +1,4 @@ +{ + "type" : "CE", + "source" : "partsel_reversed_idx3.v" +} diff --git a/ivtest/vvp_tests/partsel_reversed_idx4.json b/ivtest/vvp_tests/partsel_reversed_idx4.json new file mode 100644 index 000000000..949ce7bc2 --- /dev/null +++ b/ivtest/vvp_tests/partsel_reversed_idx4.json @@ -0,0 +1,4 @@ +{ + "type" : "CE", + "source" : "partsel_reversed_idx4.v" +} diff --git a/ivtest/vvp_tests/partsel_reversed_idx5.json b/ivtest/vvp_tests/partsel_reversed_idx5.json new file mode 100644 index 000000000..3c619c720 --- /dev/null +++ b/ivtest/vvp_tests/partsel_reversed_idx5.json @@ -0,0 +1,4 @@ +{ + "type" : "CE", + "source" : "partsel_reversed_idx5.v" +} diff --git a/ivtest/vvp_tests/partsel_reversed_idx6.json b/ivtest/vvp_tests/partsel_reversed_idx6.json new file mode 100644 index 000000000..8bfa2fff7 --- /dev/null +++ b/ivtest/vvp_tests/partsel_reversed_idx6.json @@ -0,0 +1,4 @@ +{ + "type" : "CE", + "source" : "partsel_reversed_idx6.v" +}