From 2ca1ce4e2703327dd3cd19b4f5137afda1488c47 Mon Sep 17 00:00:00 2001 From: Copilot <198982749+Copilot@users.noreply.github.com> Date: Thu, 23 Oct 2025 22:23:49 -0700 Subject: [PATCH] Fix --unroll CLI argument to properly unroll arrays instead of repeating them (#5) * Initial plan * Fix --unroll CLI argument to properly unroll arrays Updated check_is_array() method and all CPUIF implementations to correctly detect and handle unrolled array elements. When unroll=True, each array element is now treated as an individual instance instead of being repeated as an array. Co-authored-by: arnavsacheti <36746504+arnavsacheti@users.noreply.github.com> * Add comprehensive tests for --unroll functionality Created tests to verify that array elements are correctly unrolled into individual instances when --unroll flag is used. Also fixed conftest.py fixture to keep temp files for later reference. Co-authored-by: arnavsacheti <36746504+arnavsacheti@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: arnavsacheti <36746504+arnavsacheti@users.noreply.github.com> --- .../cpuif/apb3/apb3_cpuif.py | 13 +- .../cpuif/apb3/apb3_cpuif_flat.py | 10 +- .../cpuif/apb4/apb4_cpuif.py | 13 +- .../cpuif/apb4/apb4_cpuif_flat.py | 10 +- .../cpuif/axi4lite/axi4_lite_cpuif.py | 13 +- src/peakrdl_busdecoder/cpuif/base_cpuif.py | 6 +- tests/unit/conftest.py | 3 +- tests/unit/test_unroll.py | 163 ++++++++++++++++++ 8 files changed, 209 insertions(+), 22 deletions(-) create mode 100644 tests/unit/test_unroll.py diff --git a/src/peakrdl_busdecoder/cpuif/apb3/apb3_cpuif.py b/src/peakrdl_busdecoder/cpuif/apb3/apb3_cpuif.py index 3602404..1e30e52 100644 --- a/src/peakrdl_busdecoder/cpuif/apb3/apb3_cpuif.py +++ b/src/peakrdl_busdecoder/cpuif/apb3/apb3_cpuif.py @@ -12,11 +12,16 @@ class APB3Cpuif(BaseCpuif): def _port_declaration(self, child: AddressableNode) -> str: base = f"apb3_intf.master m_apb_{child.inst_name}" - if child.array_dimensions is None: - return base + + # When unrolled, current_idx is set - append it to the name if child.current_idx is not None: - return f"{base}_{'_'.join(map(str, child.current_idx))} {''.join(f'[{dim}]' for dim in child.array_dimensions)}" - return f"{base} {''.join(f'[{dim}]' for dim in child.array_dimensions)}" + base = f"{base}_{'_'.join(map(str, child.current_idx))}" + + # Only add array dimensions if this should be treated as an array + if self.check_is_array(child): + return f"{base} {''.join(f'[{dim}]' for dim in child.array_dimensions)}" + + return base @property def port_declaration(self) -> str: diff --git a/src/peakrdl_busdecoder/cpuif/apb3/apb3_cpuif_flat.py b/src/peakrdl_busdecoder/cpuif/apb3/apb3_cpuif_flat.py index 1402e77..82804e3 100644 --- a/src/peakrdl_busdecoder/cpuif/apb3/apb3_cpuif_flat.py +++ b/src/peakrdl_busdecoder/cpuif/apb3/apb3_cpuif_flat.py @@ -54,11 +54,13 @@ class APB3CpuifFlat(BaseCpuif): # Master signal base = f"m_apb_{node.inst_name}" - if not node.is_array: + if not self.check_is_array(node): + # Not an array or an unrolled element + if node.current_idx is not None: + # This is a specific instance of an unrolled array + return f"{base}_{signal}_{'_'.join(map(str, node.current_idx))}" return f"{base}_{signal}" - if node.current_idx is not None: - # This is a specific instance of an array - return f"{base}_{signal}_{'_'.join(map(str, node.current_idx))}" + # Is an array if idx is not None: return f"{base}_{signal}[{idx}]" return f"{base}_{signal}[N_{node.inst_name.upper()}S]" diff --git a/src/peakrdl_busdecoder/cpuif/apb4/apb4_cpuif.py b/src/peakrdl_busdecoder/cpuif/apb4/apb4_cpuif.py index 49c59ef..0b86eb0 100644 --- a/src/peakrdl_busdecoder/cpuif/apb4/apb4_cpuif.py +++ b/src/peakrdl_busdecoder/cpuif/apb4/apb4_cpuif.py @@ -12,11 +12,16 @@ class APB4Cpuif(BaseCpuif): def _port_declaration(self, child: AddressableNode) -> str: base = f"apb4_intf.master m_apb_{child.inst_name}" - if child.array_dimensions is None: - return base + + # When unrolled, current_idx is set - append it to the name if child.current_idx is not None: - return f"{base}_{'_'.join(map(str, child.current_idx))} {''.join(f'[{dim}]' for dim in child.array_dimensions)}" - return f"{base} {''.join(f'[{dim}]' for dim in child.array_dimensions)}" + base = f"{base}_{'_'.join(map(str, child.current_idx))}" + + # Only add array dimensions if this should be treated as an array + if self.check_is_array(child): + return f"{base} {''.join(f'[{dim}]' for dim in child.array_dimensions)}" + + return base @property def port_declaration(self) -> str: diff --git a/src/peakrdl_busdecoder/cpuif/apb4/apb4_cpuif_flat.py b/src/peakrdl_busdecoder/cpuif/apb4/apb4_cpuif_flat.py index bbce904..4793ff8 100644 --- a/src/peakrdl_busdecoder/cpuif/apb4/apb4_cpuif_flat.py +++ b/src/peakrdl_busdecoder/cpuif/apb4/apb4_cpuif_flat.py @@ -58,11 +58,13 @@ class APB4CpuifFlat(BaseCpuif): # Master signal base = f"m_apb_{node.inst_name}" - if not node.is_array: + if not self.check_is_array(node): + # Not an array or an unrolled element + if node.current_idx is not None: + # This is a specific instance of an unrolled array + return f"{base}_{signal}_{'_'.join(map(str, node.current_idx))}" return f"{base}_{signal}" - if node.current_idx is not None: - # This is a specific instance of an array - return f"{base}_{signal}_{'_'.join(map(str, node.current_idx))}" + # Is an array if idx is not None: return f"{base}_{signal}[{idx}]" return f"{base}_{signal}[N_{node.inst_name.upper()}S]" diff --git a/src/peakrdl_busdecoder/cpuif/axi4lite/axi4_lite_cpuif.py b/src/peakrdl_busdecoder/cpuif/axi4lite/axi4_lite_cpuif.py index e6d245e..d7d1a72 100644 --- a/src/peakrdl_busdecoder/cpuif/axi4lite/axi4_lite_cpuif.py +++ b/src/peakrdl_busdecoder/cpuif/axi4lite/axi4_lite_cpuif.py @@ -12,11 +12,16 @@ class AXI4LiteCpuif(BaseCpuif): def _port_declaration(self, child: AddressableNode) -> str: base = f"axi4lite_intf.master m_axil_{child.inst_name}" - if child.array_dimensions is None: - return base + + # When unrolled, current_idx is set - append it to the name if child.current_idx is not None: - return f"{base}_{'_'.join(map(str, child.current_idx))} {''.join(f'[{dim}]' for dim in child.array_dimensions)}" - return f"{base} {''.join(f'[{dim}]' for dim in child.array_dimensions)}" + base = f"{base}_{'_'.join(map(str, child.current_idx))}" + + # Only add array dimensions if this should be treated as an array + if self.check_is_array(child): + return f"{base} {''.join(f'[{dim}]' for dim in child.array_dimensions)}" + + return base @property def port_declaration(self) -> str: diff --git a/src/peakrdl_busdecoder/cpuif/base_cpuif.py b/src/peakrdl_busdecoder/cpuif/base_cpuif.py index 65dec48..641b848 100644 --- a/src/peakrdl_busdecoder/cpuif/base_cpuif.py +++ b/src/peakrdl_busdecoder/cpuif/base_cpuif.py @@ -71,7 +71,11 @@ class BaseCpuif: raise RuntimeError def check_is_array(self, node: AddressableNode) -> bool: - return node.is_array and not self.unroll + # When unrolling is enabled, children(unroll=True) returns individual + # array elements with current_idx set. These should NOT be treated as arrays. + if self.unroll and hasattr(node, 'current_idx') and node.current_idx is not None: + return False + return node.is_array def get_implementation(self) -> str: class_dir = self._get_template_path_class_dir() diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 5f526fe..100283b 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -35,7 +35,8 @@ def compile_rdl(tmp_path: Path): for include_path in include_paths or (): compiler.add_include_path(str(include_path)) - with NamedTemporaryFile("w", suffix=".rdl", dir=tmp_path) as tmp_file: + # Use delete=False to keep the file around after closing + with NamedTemporaryFile("w", suffix=".rdl", dir=tmp_path, delete=False) as tmp_file: tmp_file.write(source) tmp_file.flush() diff --git a/tests/unit/test_unroll.py b/tests/unit/test_unroll.py new file mode 100644 index 0000000..4aa8e92 --- /dev/null +++ b/tests/unit/test_unroll.py @@ -0,0 +1,163 @@ +"""Test the --unroll CLI argument functionality.""" + +from pathlib import Path +from tempfile import TemporaryDirectory + +import pytest + +from peakrdl_busdecoder import BusDecoderExporter +from peakrdl_busdecoder.cpuif.apb3 import APB3Cpuif +from peakrdl_busdecoder.cpuif.apb4 import APB4Cpuif + + +@pytest.fixture +def sample_rdl(compile_rdl): + """Create a simple RDL design with an array.""" + rdl_source = """ + addrmap top { + reg my_reg { + field { + sw=rw; + hw=r; + } data[31:0]; + }; + + my_reg regs[4] @ 0x0 += 0x4; + }; + """ + return compile_rdl(rdl_source) + + +def test_unroll_disabled_creates_array_interface(sample_rdl): + """Test that with unroll=False, array nodes are kept as arrays.""" + with TemporaryDirectory() as tmpdir: + exporter = BusDecoderExporter() + exporter.export( + sample_rdl.top, + tmpdir, + cpuif_cls=APB4Cpuif, + cpuif_unroll=False, + ) + + # Read the generated module + module_file = Path(tmpdir) / "top.sv" + content = module_file.read_text() + + # Should have a single array interface with [4] dimension + assert "m_apb_regs [4]" in content + + # Should have a parameter for array size + assert "N_REGSS = 4" in content + + # Should NOT have individual indexed interfaces + assert "m_apb_regs_0" not in content + assert "m_apb_regs_1" not in content + assert "m_apb_regs_2" not in content + assert "m_apb_regs_3" not in content + + +def test_unroll_enabled_creates_individual_interfaces(sample_rdl): + """Test that with unroll=True, array elements are unrolled into separate instances.""" + with TemporaryDirectory() as tmpdir: + exporter = BusDecoderExporter() + exporter.export( + sample_rdl.top, + tmpdir, + cpuif_cls=APB4Cpuif, + cpuif_unroll=True, + ) + + # Read the generated module + module_file = Path(tmpdir) / "top.sv" + content = module_file.read_text() + + # Should have individual interfaces without array dimensions + assert "m_apb_regs_0," in content or "m_apb_regs_0\n" in content + assert "m_apb_regs_1," in content or "m_apb_regs_1\n" in content + assert "m_apb_regs_2," in content or "m_apb_regs_2\n" in content + assert "m_apb_regs_3" in content + + # Should NOT have array interface + assert "m_apb_regs [4]" not in content + + # Should NOT have individual interfaces with array dimensions (the bug we're fixing) + assert "m_apb_regs_0 [4]" not in content + assert "m_apb_regs_1 [4]" not in content + assert "m_apb_regs_2 [4]" not in content + assert "m_apb_regs_3 [4]" not in content + + # Should NOT have array size parameter when unrolled + assert "N_REGSS" not in content + + +def test_unroll_with_apb3(sample_rdl): + """Test that unroll works correctly with APB3 interface.""" + with TemporaryDirectory() as tmpdir: + exporter = BusDecoderExporter() + exporter.export( + sample_rdl.top, + tmpdir, + cpuif_cls=APB3Cpuif, + cpuif_unroll=True, + ) + + # Read the generated module + module_file = Path(tmpdir) / "top.sv" + content = module_file.read_text() + + # Should have individual APB3 interfaces + assert "m_apb_regs_0," in content or "m_apb_regs_0\n" in content + assert "m_apb_regs_1," in content or "m_apb_regs_1\n" in content + assert "m_apb_regs_2," in content or "m_apb_regs_2\n" in content + assert "m_apb_regs_3" in content + + # Should NOT have array dimensions on unrolled interfaces + assert "m_apb_regs_0 [4]" not in content + + +@pytest.fixture +def multidim_array_rdl(compile_rdl): + """Create an RDL design with a multi-dimensional array.""" + rdl_source = """ + addrmap top { + reg my_reg { + field { + sw=rw; + hw=r; + } data[31:0]; + }; + + my_reg matrix[2][3] @ 0x0 += 0x4; + }; + """ + return compile_rdl(rdl_source) + + +def test_unroll_multidimensional_array(multidim_array_rdl): + """Test that unroll works correctly with multi-dimensional arrays.""" + with TemporaryDirectory() as tmpdir: + exporter = BusDecoderExporter() + exporter.export( + multidim_array_rdl.top, + tmpdir, + cpuif_cls=APB4Cpuif, + cpuif_unroll=True, + ) + + # Read the generated module + module_file = Path(tmpdir) / "top.sv" + content = module_file.read_text() + + # Should have individual interfaces for each element in the 2x3 array + # Format should be m_apb_matrix_0_0, m_apb_matrix_0_1, ..., m_apb_matrix_1_2 + assert "m_apb_matrix_0_0" in content + assert "m_apb_matrix_0_1" in content + assert "m_apb_matrix_0_2" in content + assert "m_apb_matrix_1_0" in content + assert "m_apb_matrix_1_1" in content + assert "m_apb_matrix_1_2" in content + + # Should NOT have array dimensions on any of the unrolled interfaces + for i in range(2): + for j in range(3): + assert f"m_apb_matrix_{i}_{j} [" not in content