Merge pull request #5066 from YosysHQ/george/opt_expr_shr_sign

opt_expr: fix sign extension for shifts

kernel/celledges.cc

@@ -247,7 +247,7 @@ void shift_op(AbstractCellEdgesDatabase *db, RTLIL::Cell *cell)
 				db->add_edge(cell, ID::A, a_width - 1, ID::Y, i, -1);
 		}
 
-		for (int k = 0; k < b_width; k++) {
+		for (int k = 0; k < b_width_capped; k++) {
 			// left shifts
 			if (cell->type.in(ID($shl), ID($sshl))) {
 				if (a_width == 1 && is_signed) {
@@ -268,7 +268,7 @@ void shift_op(AbstractCellEdgesDatabase *db, RTLIL::Cell *cell)
 					bool shift_in_bulk = i < a_width - 1;
 					// can we jump into the zero-padding by toggling B[k]?
 					bool zpad_jump = (((y_width - i) & ((1 << (k + 1)) - 1)) != 0 \
-									&& (((y_width - i) & ~(1 << k)) < (1 << b_width)));
+									&& (((y_width - i) & ~(1 << k)) < (1 << b_width_capped)));
 
 					if (shift_in_bulk || (cell->type.in(ID($shr), ID($shift), ID($shiftx)) && zpad_jump))
 						db->add_edge(cell, ID::B, k, ID::Y, i, -1);
@@ -279,7 +279,7 @@ void shift_op(AbstractCellEdgesDatabase *db, RTLIL::Cell *cell)
 			// bidirectional shifts (positive B shifts right, negative left)
 			} else if (cell->type.in(ID($shift), ID($shiftx)) && is_b_signed) {
 				if (is_signed) {
-					if (k != b_width - 1) {
+					if (k != b_width_capped - 1) {
 						bool r_shift_in_bulk = i < a_width - 1;
 						// assuming B is positive, can we jump into the upper zero-padding by toggling B[k]?
 						bool r_zpad_jump = (((y_width - i) & ((1 << (k + 1)) - 1)) != 0 \

passes/opt/opt_expr.cc

@@ -1315,13 +1315,14 @@ skip_fine_alu:
 			RTLIL::SigSpec sig_a = assign_map(cell->getPort(ID::A));
 			RTLIL::SigSpec sig_y(cell->type == ID($shiftx) ? RTLIL::State::Sx : RTLIL::State::S0, cell->getParam(ID::Y_WIDTH).as_int());
 
-			// Limit indexing to the size of a, which is behaviourally identical (result is all 0)
-			// and avoids integer overflow of i + shift_bits when e.g. ID::B == INT_MAX
-			shift_bits = min(shift_bits, GetSize(sig_a));
-
 			if (cell->type != ID($shiftx) && GetSize(sig_a) < GetSize(sig_y))
 				sig_a.extend_u0(GetSize(sig_y), cell->getParam(ID::A_SIGNED).as_bool());
 
+			// Limit indexing to the size of a, which is behaviourally identical (result is all 0)
+			// and avoids integer overflow of i + shift_bits when e.g. ID::B == INT_MAX.
+			// We do this after sign-extending a so this accounts for the output size
+			shift_bits = min(shift_bits, GetSize(sig_a));
+
 			for (int i = 0; i < GetSize(sig_y); i++) {
 				int idx = i + shift_bits;
 				if (0 <= idx && idx < GetSize(sig_a))

tests/opt/opt_expr_shift.ys

@@ -0,0 +1,50 @@
+# Testing edge cases where ports are signed/have different widths/shift amounts
+# greater than the size
+
+read_verilog <<EOT
+module top (
+	input  wire        [3:0]  in_u,
+	input  wire signed [3:0]  in_s,
+	output wire        [7:0]  shl_uu,
+	output wire signed [7:0]  shl_us,
+	output wire        [7:0]  shl_su,
+	output wire signed [7:0]  shl_ss,
+	output wire        [7:0]  shr_uu,
+	output wire signed [7:0]  shr_us,
+	output wire        [7:0]  shr_su,
+	output wire signed [7:0]  shr_ss,
+	output wire        [7:0] sshl_uu,
+	output wire signed [7:0] sshl_us,
+	output wire        [7:0] sshl_su,
+	output wire signed [7:0] sshl_ss,
+	output wire        [7:0] sshr_uu,
+	output wire signed [7:0] sshr_us,
+	output wire        [7:0] sshr_su,
+	output wire signed [7:0] sshr_ss
+);
+	assign  shl_uu = in_u << 20;
+	assign  shl_us = in_u << 20;
+	assign  shl_su = in_s << 20;
+	assign  shl_ss = in_s << 20;
+	assign  shr_uu = in_u >> 20;
+	assign  shr_us = in_u >> 20;
+	assign  shr_su = in_s >> 20;
+	assign  shr_ss = in_s >> 20;
+	assign sshl_uu = in_u <<< 20;
+	assign sshl_us = in_u <<< 20;
+	assign sshl_su = in_s <<< 20;
+	assign sshl_ss = in_s <<< 20;
+	assign sshr_uu = in_u >>> 20;
+	assign sshr_us = in_u >>> 20;
+	assign sshr_su = in_s >>> 20;
+	assign sshr_ss = in_s >>> 20;
+endmodule
+EOT
+
+equiv_opt opt_expr
+
+design -load postopt
+select -assert-none t:$shl
+select -assert-none t:$shr
+select -assert-none t:$sshl
+select -assert-none t:$sshr