Merge pull request #1059 from larsclausen/invalid-partsel

Detect reversed part select on inner dimensions

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.

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<<":"<<lsb<<"]"
-		       << " is reversed." << endl;
-		  des->errors += 1;
-		  return false;
-	    }
       }
 
+      if (moff < loff) {
+	    cerr << get_fileline() << ": error: part select "
+		 << reg->name() << "[" << msb<<":"<<lsb<<"]"
+		 << " is reversed." << endl;
+	    des->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;
       }
 

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;
 	  }

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

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

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

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

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

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

ivtest/regress-vvp.list

@@ -95,6 +95,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

ivtest/vvp_tests/partsel_reversed_idx1.json

@@ -0,0 +1,4 @@
+{
+    "type"          : "CE",
+    "source"        : "partsel_reversed_idx1.v"
+}

ivtest/vvp_tests/partsel_reversed_idx2.json

@@ -0,0 +1,4 @@
+{
+    "type"          : "CE",
+    "source"        : "partsel_reversed_idx2.v"
+}

ivtest/vvp_tests/partsel_reversed_idx3.json

@@ -0,0 +1,4 @@
+{
+    "type"          : "CE",
+    "source"        : "partsel_reversed_idx3.v"
+}

ivtest/vvp_tests/partsel_reversed_idx4.json

@@ -0,0 +1,4 @@
+{
+    "type"          : "CE",
+    "source"        : "partsel_reversed_idx4.v"
+}

ivtest/vvp_tests/partsel_reversed_idx5.json

@@ -0,0 +1,4 @@
+{
+    "type"          : "CE",
+    "source"        : "partsel_reversed_idx5.v"
+}

ivtest/vvp_tests/partsel_reversed_idx6.json

@@ -0,0 +1,4 @@
+{
+    "type"          : "CE",
+    "source"        : "partsel_reversed_idx6.v"
+}