Repository: amaranth-lang/amaranth (python)

Issue:
struggling to understand source of an error
https://github.com/q3w3e3/amaranthSieve/blob/e1dbdbd0e990d9dbdeb7c2e92550d1058a265ef5/src/amaranth_sieve/sieve2.py

```
% pdm run test
================================================ test session starts =================================================
platform darwin -- Python 3.12.4, pytest-8.2.2, pluggy-1.5.0
rootdir: /Users/q3w3e3/Stuff/amaranthSieve
configfile: pyproject.toml
collected 1 item                                                                                                     

tests/test_sieve.py F                                                                                          [100%]

====================================================== FAILURES ======================================================
____________________________________________________ test_sieve2 _____________________________________________________

    def test_sieve2():
        sieve2 = Sieve2()
    
        async def testbench(ctx):
            while ctx.get(~sieve2.done):
                await ctx.tick()
    
    
        sim2 = Simulator(sieve2)
        sim2.add_clock(period=1.667e-7)
        sim2.add_testbench(testbench)
        with sim2.write_vcd("sieve2.vcd"):
>           sim2.run_until(1.667e-7 * 200)

tests/test_sieve.py:18: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
.venv/lib/python3.12/site-packages/amaranth/sim/core.py:313: in run_until
    self.advance()
.venv/lib/python3.12/site-packages/amaranth/sim/core.py:330: in advance
    return self._engine.advance()
.venv/lib/python3.12/site-packages/amaranth/sim/pysim.py:692: in advance
    self.step_design()
.venv/lib/python3.12/site-packages/amaranth/sim/pysim.py:683: in step_design
    vcd_writer.update_memory(now_plus_deltas, change.state.memory,
.venv/lib/python3.12/site-packages/amaranth/sim/pysim.py:259: in update_memory
    self.vcd_writer.change(vcd_var, timestamp, var_value)
.venv/lib/python3.12/site-packages/vcd/writer.py:353: in change
    val_str = var.format_value(value, self._check_values)
.venv/lib/python3.12/site-packages/vcd/writer.py:669: in format_value
    value_str = _format_scalar_value(value, self.size, check)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

value = -170, size = 8, check = True

    def _format_scalar_value(value: ScalarValue, size: int, check: bool) -> str:
        if isinstance(value, int):
            max_val = 1 << size
            if check and (-value > (max_val >> 1) or value >= max_val):
>               raise ValueError(f'Value ({value}) not representable in {size} bits')
E               ValueError: Value (-170) not representable in 8 bits

.venv/lib/python3.12/site-packages/vcd/writer.py:734: ValueError
============================================== short test summary info ===============================================
FAILED tests/test_sieve.py::test_sieve2 - ValueError: Value (-170) not representable in 8 bits
================================================= 1 failed in 0.10s ==================================================

```

Pull request:
hdl.mem: mask initial value to shape.
Fixes #1492.

Patch:
diff --git a/amaranth/hdl/_ast.py b/amaranth/hdl/_ast.py
index d99cd72..3f87e9e 100644
--- a/amaranth/hdl/_ast.py
+++ b/amaranth/hdl/_ast.py
@@ -1946,6 +1946,57 @@ class _SignalMeta(ABCMeta):
         return signal
 
 
+# also used for MemoryData.Init
+def _get_init_value(init, shape, what="signal"):
+    orig_init = init
+    orig_shape = shape
+    shape = Shape.cast(shape)
+    if isinstance(orig_shape, ShapeCastable):
+        try:
+            init = Const.cast(orig_shape.const(init))
+        except Exception:
+            raise TypeError(f"Initial value must be a constant initializer of {orig_shape!r}")
+        if init.shape() != Shape.cast(shape):
+            raise ValueError(f"Constant returned by {orig_shape!r}.const() must have the shape "
+                             f"that it casts to, {shape!r}, and not {init.shape()!r}")
+        return init.value
+    else:
+        if init is None:
+            init = 0
+        try:
+            init = Const.cast(init)
+        except TypeError:
+            raise TypeError("Initial value must be a constant-castable expression, not {!r}"
+                            .format(orig_init))
+        # Avoid false positives for all-zeroes and all-ones
+        if orig_init is not None and not (isinstance(orig_init, int) and orig_init in (0, -1)):
+            if init.shape().signed and not shape.signed:
+                warnings.warn(
+                    message=f"Initial value {orig_init!r} is signed, "
+                            f"but the {what} shape is {shape!r}",
+                    category=SyntaxWarning,
+                    stacklevel=2)
+            elif (init.shape().width > shape.width or
+                  init.shape().width == shape.width and
+                    shape.signed and not init.shape().signed):
+                warnings.warn(
+                    message=f"Initial value {orig_init!r} will be truncated to "
+                            f"the {what} shape {shape!r}",
+                    category=SyntaxWarning,
+                    stacklevel=2)
+
+        if isinstance(orig_shape, range) and orig_init is not None and orig_init not in orig_shape:
+            if orig_init == orig_shape.stop:
+                raise SyntaxError(
+                    f"Initial value {orig_init!r} equals the non-inclusive end of the {what} "
+                    f"shape {orig_shape!r}; this is likely an off-by-one error")
+            else:
+                raise SyntaxError(
+                    f"Initial value {orig_init!r} is not within the {what} shape {orig_shape!r}")
+
+        return Const(init.value, shape).value
+
+
 @final
 class Signal(Value, DUID, metaclass=_SignalMeta):
     """A varying integer value.
@@ -2016,54 +2067,9 @@ class Signal(Value, DUID, metaclass=_SignalMeta):
                           DeprecationWarning, stacklevel=2)
             init = reset
 
-        orig_init = init
-        if isinstance(orig_shape, ShapeCastable):
-            try:
-                init = Const.cast(orig_shape.const(init))
-            except Exception:
-                raise TypeError("Initial value must be a constant initializer of {!r}"
-                                .format(orig_shape))
-            if init.shape() != Shape.cast(orig_shape):
-                raise ValueError("Constant returned by {!r}.const() must have the shape that "
-                                 "it casts to, {!r}, and not {!r}"
-                                 .format(orig_shape, Shape.cast(orig_shape),
-                                         init.shape()))
-        else:
-            if init is None:
-                init = 0
-            try:
-                init = Const.cast(init)
-            except TypeError:
-                raise TypeError("Initial value must be a constant-castable expression, not {!r}"
-                                .format(orig_init))
-        # Avoid false positives for all-zeroes and all-ones
-        if orig_init is not None and not (isinstance(orig_init, int) and orig_init in (0, -1)):
-            if init.shape().signed and not self._signed:
-                warnings.warn(
-                    message="Initial value {!r} is signed, but the signal shape is {!r}"
-                            .format(orig_init, shape),
-                    category=SyntaxWarning,
-                    stacklevel=2)
-            elif (init.shape().width > self._width or
-                  init.shape().width == self._width and
-                    self._signed and not init.shape().signed):
-                warnings.warn(
-                    message="Initial value {!r} will be truncated to the signal shape {!r}"
-                            .format(orig_init, shape),
-                    category=SyntaxWarning,
-                    stacklevel=2)
-        self._init = Const(init.value, shape).value
+        self._init = _get_init_value(init, unsigned(1) if orig_shape is None else orig_shape)
         self._reset_less = bool(reset_less)
 
-        if isinstance(orig_shape, range) and orig_init is not None and orig_init not in orig_shape:
-            if orig_init == orig_shape.stop:
-                raise SyntaxError(
-                    f"Initial value {orig_init!r} equals the non-inclusive end of the signal "
-                    f"shape {orig_shape!r}; this is likely an off-by-one error")
-            else:
-                raise SyntaxError(
-                    f"Initial value {orig_init!r} is not within the signal shape {orig_shape!r}")
-
         self._attrs = OrderedDict(() if attrs is None else attrs)
 
         if isinstance(orig_shape, ShapeCastable):
diff --git a/amaranth/hdl/_mem.py b/amaranth/hdl/_mem.py
index 0554f73..ab34855 100644
--- a/amaranth/hdl/_mem.py
+++ b/amaranth/hdl/_mem.py
@@ -3,6 +3,7 @@ from collections.abc import MutableSequence
 
 from .. import tracer
 from ._ast import *
+from ._ast import _get_init_value
 from ._ir import Fragment, AlreadyElaborated
 from ..utils import ceil_log2
 from .._utils import final
@@ -105,10 +106,11 @@ class MemoryData:
                 for actual_index, actual_value in zip(indices, value):
                     self[actual_index] = actual_value
             else:
+                raw = _get_init_value(value, self._shape, "memory")
                 if isinstance(self._shape, ShapeCastable):
-                    self._raw[index] = Const.cast(Const(value, self._shape)).value
+                    self._raw[index] = raw
                 else:
-                    value = operator.index(value)
+                    value = raw
                     # self._raw[index] assigned by the following line
                 self._elems[index] = value
 
diff --git a/amaranth/sim/pysim.py b/amaranth/sim/pysim.py
index 2d9562d..ddfb8ad 100644
--- a/amaranth/sim/pysim.py
+++ b/amaranth/sim/pysim.py
@@ -392,10 +392,11 @@ class _PyMemoryChange:
 
 
 class _PyMemoryState(BaseMemoryState):
-    __slots__ = ("memory", "data", "write_queue", "wakers", "pending")
+    __slots__ = ("memory", "shape", "data", "write_queue", "wakers", "pending")
 
     def __init__(self, memory, pending):
         self.memory  = memory
+        self.shape   = Shape.cast(memory.shape)
         self.pending = pending
         self.wakers  = list()
         self.reset()
@@ -419,6 +420,11 @@ class _PyMemoryState(BaseMemoryState):
                 self.write_queue[addr] = self.data[addr]
             if mask is not None:
                 value = (value & mask) | (self.write_queue[addr] & ~mask)
+            if self.shape.signed:
+                if value & (1 << (self.shape.width - 1)):
+                    value |= -1 << (self.shape.width)
+                else:
+                    value &= (1 << (self.shape.width)) - 1
             self.write_queue[addr] = value
             self.pending.add(self)
 


Test patch:
diff --git a/tests/test_hdl_mem.py b/tests/test_hdl_mem.py
index 504e598..fb45965 100644
--- a/tests/test_hdl_mem.py
+++ b/tests/test_hdl_mem.py
@@ -31,3 +31,29 @@ class MemoryDataTestCase(FHDLTestCase):
         with self.assertRaisesRegex(ValueError,
                 r"^Value \(memory-row \(memory-data data\) 0\) can only be used in simulator processes$"):
             m.d.comb += data[0].eq(1)
+
+
+class InitTestCase(FHDLTestCase):
+    def test_ones(self):
+        init = MemoryData.Init([-1, 12], shape=8, depth=2)
+        self.assertEqual(list(init), [0xff, 12])
+        init = MemoryData.Init([-1, -12], shape=signed(8), depth=2)
+        self.assertEqual(list(init), [-1, -12])
+
+    def test_trunc(self):
+        with self.assertWarnsRegex(SyntaxWarning,
+                r"^Initial value -2 is signed, but the memory shape is unsigned\(8\)$"):
+            init = MemoryData.Init([-2, 12], shape=8, depth=2)
+        self.assertEqual(list(init), [0xfe, 12])
+        with self.assertWarnsRegex(SyntaxWarning,
+                r"^Initial value 258 will be truncated to the memory shape unsigned\(8\)$"):
+            init = MemoryData.Init([258, 129], shape=8, depth=2)
+        self.assertEqual(list(init), [2, 129])
+        with self.assertWarnsRegex(SyntaxWarning,
+                r"^Initial value 128 will be truncated to the memory shape signed\(8\)$"):
+            init = MemoryData.Init([128], shape=signed(8), depth=1)
+        self.assertEqual(list(init), [-128])
+        with self.assertWarnsRegex(SyntaxWarning,
+                r"^Initial value -129 will be truncated to the memory shape signed\(8\)$"):
+            init = MemoryData.Init([-129], shape=signed(8), depth=1)
+        self.assertEqual(list(init), [127])
diff --git a/tests/test_lib_memory.py b/tests/test_lib_memory.py
index 3700613..ba828a8 100644
--- a/tests/test_lib_memory.py
+++ b/tests/test_lib_memory.py
@@ -329,7 +329,7 @@ class MemoryTestCase(FHDLTestCase):
             memory.Memory(shape="a", depth=3, init=[])
         with self.assertRaisesRegex(TypeError,
                 (r"^Memory initialization value at address 1: "
-                    r"'str' object cannot be interpreted as an integer$")):
+                    r"Initial value must be a constant-castable expression, not '0'$")):
             memory.Memory(shape=8, depth=4, init=[1, "0"])
         with self.assertRaisesRegex(ValueError,
                 r"^Either 'data' or 'shape' needs to be given$"):
@@ -373,7 +373,7 @@ class MemoryTestCase(FHDLTestCase):
     def test_init_set_wrong(self):
         m = memory.Memory(shape=8, depth=4, init=[])
         with self.assertRaisesRegex(TypeError,
-                r"^'str' object cannot be interpreted as an integer$"):
+                r"^Initial value must be a constant-castable expression, not 'a'$"):
             m.init[0] = "a"
         m = memory.Memory(shape=MyStruct, depth=4, init=[])
         # underlying TypeError message differs between PyPy and CPython
diff --git a/tests/test_sim.py b/tests/test_sim.py
index 419e504..45e84d1 100644
--- a/tests/test_sim.py
+++ b/tests/test_sim.py
@@ -1418,15 +1418,20 @@ class SimulatorIntegrationTestCase(FHDLTestCase):
         mem1 = MemoryData(shape=8, depth=4, init=[1, 2, 3])
         mem2 = MemoryData(shape=MyEnum, depth=4, init=[MyEnum.A, MyEnum.B, MyEnum.C])
         mem3 = MemoryData(shape=data.StructLayout({"a": signed(3), "b": 2}), depth=4, init=[{"a": 2, "b": 1}])
+        mem4 = MemoryData(shape=signed(8), depth=4, init=[1, -2, 3])
 
         async def testbench(ctx):
             await ctx.delay(1e-6)
             ctx.set(mem1[0], 4)
             ctx.set(mem2[3], MyEnum.C)
             ctx.set(mem3[2], {"a": -1, "b": 2})
+            ctx.set(mem4[1][4:], 0)
+            ctx.set(mem4[3][7], 1)
             await ctx.delay(1e-6)
+            self.assertEqual(ctx.get(mem4[1]), 0xe)
+            self.assertEqual(ctx.get(mem4[3]), -128)
 
-        with self.assertSimulation(Module(), traces=[mem1, mem2, mem3]) as sim:
+        with self.assertSimulation(Module(), traces=[mem1, mem2, mem3, mem4]) as sim:
             sim.add_testbench(testbench)
 
     def test_multiple_modules(self):