From 2e6ae1c7c047a050982e24af964ee98d6fb82f91 Mon Sep 17 00:00:00 2001 From: whitequark Date: Sat, 11 Dec 2021 08:52:14 +0000 Subject: [PATCH] hdl.ast: support division and modulo with negative divisor. Fixes #621. This commit bumps the Yosys version requirement to >=0.10. --- docs/lang.rst | 14 ++++++-------- nmigen/back/cxxrtl.py | 3 +-- nmigen/back/rtlil.py | 31 ++----------------------------- nmigen/back/verilog.py | 21 ++------------------- nmigen/hdl/ast.py | 20 ++++---------------- tests/test_hdl_ast.py | 18 ++++++------------ tests/test_sim.py | 14 ++++++++++++++ 7 files changed, 35 insertions(+), 86 deletions(-) diff --git a/docs/lang.rst b/docs/lang.rst index 0724835..6e847cd 100644 --- a/docs/lang.rst +++ b/docs/lang.rst @@ -421,19 +421,17 @@ While arithmetic computations never result in an overflow, :ref:`assigning = (0, 9, 3468)) + yosys = find_yosys(lambda ver: ver >= (0, 10)) script = [] if black_boxes is not None: for box_name, box_source in black_boxes.items(): script.append("read_ilang <>"): "$sshr", @@ -825,9 +825,6 @@ def _convert_fragment(builder, fragment, name_map, hierarchy): lhs_compiler = _LHSValueCompiler(compiler_state) stmt_compiler = _StatementCompiler(compiler_state, rhs_compiler, lhs_compiler) - verilog_trigger = None - verilog_trigger_sync_emitted = False - # If the fragment is completely empty, add a dummy wire to it, or Yosys will interpret # it as a black box by default (when read as Verilog). if not fragment.ports and not fragment.statements and not fragment.subfragments: @@ -942,24 +939,6 @@ def _convert_fragment(builder, fragment, name_map, hierarchy): stmt_compiler._wrap_assign = False stmt_compiler(group_stmts) - # Verilog `always @*` blocks will not run if `*` does not match anything, i.e. - # if the implicit sensitivity list is empty. We check this while translating, - # by looking for any signals on RHS. If there aren't any, we add some logic - # whose only purpose is to trigger Verilog simulators when it converts - # through RTLIL and to Verilog, by populating the sensitivity list. - # - # Unfortunately, while this workaround allows true (event-driven) Verilog - # simulators to work properly, and is universally ignored by synthesizers, - # Verilator rejects it. - # - # Yosys >=0.9+3468 emits a better workaround on its own, so this code can be - # removed completely once support for Yosys 0.9 is dropped. - if not stmt_compiler._has_rhs: - if verilog_trigger is None: - verilog_trigger = \ - module.wire(1, name="$verilog_initial_trigger") - case.assign(verilog_trigger, verilog_trigger) - # For every signal in the sync domain, assign \sig's initial value (which will # end up as the \init reg attribute) to the reset value. with process.sync("init") as sync: @@ -969,12 +948,6 @@ def _convert_fragment(builder, fragment, name_map, hierarchy): wire_curr, wire_next = compiler_state.resolve(signal) sync.update(wire_curr, rhs_compiler(ast.Const(signal.reset, signal.width))) - # The Verilog simulator trigger needs to change at time 0, so if we haven't - # yet done that in some process, do it. - if verilog_trigger and not verilog_trigger_sync_emitted: - sync.update(verilog_trigger, "1'0") - verilog_trigger_sync_emitted = True - # For every signal in every sync domain, assign \sig to \sig$next. The sensitivity # list, however, differs between domains: for domains with sync reset, it is # `[pos|neg]edge clk`, for sync domains with async reset it is `[pos|neg]edge clk diff --git a/nmigen/back/verilog.py b/nmigen/back/verilog.py index 181a765..b9c32d6 100644 --- a/nmigen/back/verilog.py +++ b/nmigen/back/verilog.py @@ -7,29 +7,12 @@ __all__ = ["YosysError", "convert", "convert_fragment"] def _convert_rtlil_text(rtlil_text, *, strip_internal_attrs=False, write_verilog_opts=()): # this version requirement needs to be synchronized with the one in setup.py! - yosys = find_yosys(lambda ver: ver >= (0, 9)) + yosys = find_yosys(lambda ver: ver >= (0, 10)) yosys_version = yosys.version() script = [] script.append("read_ilang <= (0, 9, 3468): - # Yosys >=0.9+3468 (since commit 128522f1) emits the workaround for the `always @*` - # initial scheduling issue on its own. - script.append("delete w:$verilog_initial_trigger") - - if yosys_version >= (0, 9, 3527): - # Yosys >=0.9+3527 (since commit 656ee70f) supports the `-nomux` option for the `proc` - # script pass. Because the individual `proc_*` passes are not a stable interface, - # `proc -nomux` is used instead, if available. - script.append("proc -nomux") - else: - # On earlier versions, use individual `proc_*` passes; this is a known range of Yosys - # versions and we know it's compatible with what nMigen does. - script.append("proc_init") - script.append("proc_arst") - script.append("proc_dff") - script.append("proc_clean") + script.append("proc -nomux") script.append("memory_collect") if strip_internal_attrs: diff --git a/nmigen/hdl/ast.py b/nmigen/hdl/ast.py index 0049c04..1bbe9f6 100644 --- a/nmigen/hdl/ast.py +++ b/nmigen/hdl/ast.py @@ -172,26 +172,13 @@ class Value(metaclass=ABCMeta): def __rmul__(self, other): return Operator("*", [other, self]) - def __check_divisor(self): - width, signed = self.shape() - if signed: - # Python's division semantics and Verilog's division semantics differ for negative - # divisors (Python uses div/mod, Verilog uses quo/rem); for now, avoid the issue - # completely by prohibiting such division operations. - raise NotImplementedError("Division by a signed value is not supported") def __mod__(self, other): - other = Value.cast(other) - other.__check_divisor() return Operator("%", [self, other]) def __rmod__(self, other): - self.__check_divisor() return Operator("%", [other, self]) def __floordiv__(self, other): - other = Value.cast(other) - other.__check_divisor() return Operator("//", [self, other]) def __rfloordiv__(self, other): - self.__check_divisor() return Operator("//", [other, self]) def __check_shamt(self): @@ -692,9 +679,10 @@ class Operator(Value): return Shape(width + 1, signed) if self.operator == "*": return Shape(a_width + b_width, a_signed or b_signed) - if self.operator in ("//", "%"): - assert not b_signed - return Shape(a_width, a_signed) + if self.operator == "//": + return Shape(a_width + b_signed, a_signed or b_signed) + if self.operator == "%": + return Shape(b_width, b_signed) if self.operator in ("<", "<=", "==", "!=", ">", ">="): return Shape(1, False) if self.operator in ("&", "^", "|"): diff --git a/tests/test_hdl_ast.py b/tests/test_hdl_ast.py index 83f6071..832e90f 100644 --- a/tests/test_hdl_ast.py +++ b/tests/test_hdl_ast.py @@ -407,31 +407,25 @@ class OperatorTestCase(FHDLTestCase): def test_mod(self): v1 = Const(0, unsigned(4)) % Const(0, unsigned(6)) self.assertEqual(repr(v1), "(% (const 4'd0) (const 6'd0))") - self.assertEqual(v1.shape(), unsigned(4)) + self.assertEqual(v1.shape(), unsigned(6)) v3 = Const(0, signed(4)) % Const(0, unsigned(4)) - self.assertEqual(v3.shape(), signed(4)) + self.assertEqual(v3.shape(), unsigned(4)) + v4 = Const(0, signed(4)) % Const(0, signed(6)) + self.assertEqual(v4.shape(), signed(6)) v5 = 10 % Const(0, 4) self.assertEqual(v5.shape(), unsigned(4)) - def test_mod_wrong(self): - with self.assertRaisesRegex(NotImplementedError, - r"^Division by a signed value is not supported$"): - Const(0, signed(4)) % Const(0, signed(6)) - def test_floordiv(self): v1 = Const(0, unsigned(4)) // Const(0, unsigned(6)) self.assertEqual(repr(v1), "(// (const 4'd0) (const 6'd0))") self.assertEqual(v1.shape(), unsigned(4)) v3 = Const(0, signed(4)) // Const(0, unsigned(4)) self.assertEqual(v3.shape(), signed(4)) + v4 = Const(0, signed(4)) // Const(0, signed(6)) + self.assertEqual(v4.shape(), signed(5)) v5 = 10 // Const(0, 4) self.assertEqual(v5.shape(), unsigned(4)) - def test_floordiv_wrong(self): - with self.assertRaisesRegex(NotImplementedError, - r"^Division by a signed value is not supported$"): - Const(0, signed(4)) // Const(0, signed(6)) - def test_and(self): v1 = Const(0, unsigned(4)) & Const(0, unsigned(6)) self.assertEqual(repr(v1), "(& (const 4'd0) (const 6'd0))") diff --git a/tests/test_sim.py b/tests/test_sim.py index e4bd5c8..a64e90f 100644 --- a/tests/test_sim.py +++ b/tests/test_sim.py @@ -116,6 +116,13 @@ class SimulatorUnitTestCase(FHDLTestCase): self.assertStatement(stmt, [C(2, 4), C(2, 4)], C(1, 8)) self.assertStatement(stmt, [C(7, 4), C(2, 4)], C(3, 8)) + def test_floordiv_neg(self): + stmt = lambda y, a, b: y.eq(a // b) + self.assertStatement(stmt, [C(-5, 4), C( 2, 4)], C(-3, 8)) + self.assertStatement(stmt, [C(-5, 4), C(-2, 4)], C( 2, 8)) + self.assertStatement(stmt, [C( 5, 4), C( 2, 4)], C( 2, 8)) + self.assertStatement(stmt, [C( 5, 4), C(-2, 4)], C(-3, 8)) + def test_mod(self): stmt = lambda y, a, b: y.eq(a % b) self.assertStatement(stmt, [C(2, 4), C(0, 4)], C(0, 8)) @@ -123,6 +130,13 @@ class SimulatorUnitTestCase(FHDLTestCase): self.assertStatement(stmt, [C(2, 4), C(2, 4)], C(0, 8)) self.assertStatement(stmt, [C(7, 4), C(2, 4)], C(1, 8)) + def test_mod_neg(self): + stmt = lambda y, a, b: y.eq(a % b) + self.assertStatement(stmt, [C(-5, 4), C( 3, 4)], C( 1, 8)) + self.assertStatement(stmt, [C(-5, 4), C(-3, 4)], C(-2, 8)) + self.assertStatement(stmt, [C( 5, 4), C( 3, 4)], C( 2, 8)) + self.assertStatement(stmt, [C( 5, 4), C(-3, 4)], C(-1, 8)) + def test_and(self): stmt = lambda y, a, b: y.eq(a & b) self.assertStatement(stmt, [C(0b1100, 4), C(0b1010, 4)], C(0b1000, 4)) -- 2.30.2