Fix decoder generation for external nested addressable components and add max-decode-depth parameter (#12)
* Initial plan * Fix bus decoder to skip external nested components Co-authored-by: arnavsacheti <36746504+arnavsacheti@users.noreply.github.com> * Optimize external children check using generator expressions Co-authored-by: arnavsacheti <36746504+arnavsacheti@users.noreply.github.com> * Add max-decode-depth CLI argument Added --max-decode-depth argument that: - Is added to CLI arguments in __peakrdl__.py - Piped into design state via ExporterKwargs and DesignStateKwargs - Used to control max depth in listener.py - All 66 tests pass including new test for the parameter 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>
This commit is contained in:
@@ -111,6 +111,15 @@ class Exporter(ExporterSubcommandPlugin):
|
||||
""",
|
||||
)
|
||||
|
||||
arg_group.add_argument(
|
||||
"--max-decode-depth",
|
||||
type=int,
|
||||
default=1,
|
||||
help="""Maximum depth for address decoder to descend into nested
|
||||
addressable components. Default is 1.
|
||||
""",
|
||||
)
|
||||
|
||||
def do_export(self, top_node: "AddrmapNode", options: "argparse.Namespace") -> None:
|
||||
cpuifs = self.get_cpuifs()
|
||||
|
||||
@@ -123,4 +132,5 @@ class Exporter(ExporterSubcommandPlugin):
|
||||
package_name=options.package_name,
|
||||
address_width=options.addr_width,
|
||||
cpuif_unroll=options.unroll,
|
||||
max_decode_depth=options.max_decode_depth,
|
||||
)
|
||||
|
||||
@@ -14,6 +14,7 @@ class DesignStateKwargs(TypedDict, total=False):
|
||||
package_name: str
|
||||
address_width: int
|
||||
cpuif_unroll: bool
|
||||
max_decode_depth: int
|
||||
|
||||
|
||||
class DesignState:
|
||||
@@ -35,6 +36,7 @@ class DesignState:
|
||||
user_addr_width: int | None = kwargs.pop("address_width", None)
|
||||
|
||||
self.cpuif_unroll: bool = kwargs.pop("cpuif_unroll", False)
|
||||
self.max_decode_depth: int = kwargs.pop("max_decode_depth", 1)
|
||||
|
||||
# ------------------------
|
||||
# Info about the design
|
||||
|
||||
@@ -27,6 +27,7 @@ class ExporterKwargs(TypedDict, total=False):
|
||||
address_width: int
|
||||
cpuif_unroll: bool
|
||||
reuse_hwif_typedefs: bool
|
||||
max_decode_depth: int
|
||||
|
||||
|
||||
if TYPE_CHECKING:
|
||||
@@ -84,6 +85,9 @@ class BusDecoderExporter:
|
||||
cpuif_unroll: bool
|
||||
Unroll arrayed addressable nodes into separate instances in the CPU
|
||||
interface. By default, arrayed nodes are kept as arrays.
|
||||
max_decode_depth: int
|
||||
Maximum depth for address decoder to descend into nested addressable
|
||||
components. By default, the decoder descends 1 level deep.
|
||||
"""
|
||||
# If it is the root node, skip to top addrmap
|
||||
if isinstance(node, RootNode):
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
from collections import deque
|
||||
|
||||
from systemrdl.node import AddressableNode
|
||||
from systemrdl.node import AddressableNode, RegNode
|
||||
from systemrdl.walker import RDLListener, WalkerAction
|
||||
|
||||
from .design_state import DesignState
|
||||
@@ -12,6 +12,20 @@ class BusDecoderListener(RDLListener):
|
||||
self._ds = ds
|
||||
self._depth = 0
|
||||
|
||||
def should_skip_node(self, node: AddressableNode) -> bool:
|
||||
"""Check if this node should be skipped (not decoded)."""
|
||||
# Check if current depth exceeds max depth
|
||||
if self._depth > self._ds.max_decode_depth:
|
||||
return True
|
||||
|
||||
# Check if this node only contains external addressable children
|
||||
if node != self._ds.top_node and not isinstance(node, RegNode):
|
||||
if any(isinstance(c, AddressableNode) for c in node.children()) and \
|
||||
all(c.external for c in node.children() if isinstance(c, AddressableNode)):
|
||||
return True
|
||||
|
||||
return False
|
||||
|
||||
def enter_AddressableComponent(self, node: AddressableNode) -> WalkerAction | None:
|
||||
if node.array_dimensions:
|
||||
assert node.array_stride is not None, "Array stride should be defined for arrayed components"
|
||||
@@ -35,8 +49,10 @@ class BusDecoderListener(RDLListener):
|
||||
|
||||
self._depth += 1
|
||||
|
||||
if self._depth > 1:
|
||||
# Check if we should skip this node's descendants
|
||||
if self.should_skip_node(node):
|
||||
return WalkerAction.SkipDescendants
|
||||
|
||||
return WalkerAction.Continue
|
||||
|
||||
def exit_AddressableComponent(self, node: AddressableNode) -> None:
|
||||
|
||||
202
tests/unit/test_external_nested.py
Normal file
202
tests/unit/test_external_nested.py
Normal file
@@ -0,0 +1,202 @@
|
||||
"""Test handling of external nested addressable components."""
|
||||
|
||||
from pathlib import Path
|
||||
from tempfile import TemporaryDirectory
|
||||
|
||||
import pytest
|
||||
|
||||
from peakrdl_busdecoder import BusDecoderExporter
|
||||
from peakrdl_busdecoder.cpuif.apb4 import APB4Cpuif
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def external_nested_rdl(compile_rdl):
|
||||
"""Create an RDL design with external nested addressable components.
|
||||
|
||||
This tests the scenario where an addrmap contains external children
|
||||
that themselves have external addressable children.
|
||||
The decoder should only generate select signals for the top-level
|
||||
external children, not their internal structure.
|
||||
"""
|
||||
rdl_source = """
|
||||
mem queue_t {
|
||||
name = "Queue";
|
||||
mementries = 1024;
|
||||
memwidth = 64;
|
||||
};
|
||||
|
||||
addrmap port_t {
|
||||
name = "Port";
|
||||
desc = "";
|
||||
|
||||
external queue_t common[3] @ 0x0 += 0x2000;
|
||||
external queue_t response @ 0x6000;
|
||||
};
|
||||
|
||||
addrmap buffer_t {
|
||||
name = "Buffer";
|
||||
desc = "";
|
||||
|
||||
port_t multicast @ 0x0;
|
||||
port_t port [16] @ 0x8000 += 0x8000;
|
||||
};
|
||||
"""
|
||||
return compile_rdl(rdl_source, top="buffer_t")
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def nested_addrmap_rdl(compile_rdl):
|
||||
"""Create an RDL design with nested non-external addrmaps for testing depth control."""
|
||||
rdl_source = """
|
||||
addrmap level2 {
|
||||
reg {
|
||||
field { sw=rw; hw=r; } data2[31:0];
|
||||
} reg2 @ 0x0;
|
||||
|
||||
reg {
|
||||
field { sw=rw; hw=r; } data2b[31:0];
|
||||
} reg2b @ 0x4;
|
||||
};
|
||||
|
||||
addrmap level1 {
|
||||
reg {
|
||||
field { sw=rw; hw=r; } data1[31:0];
|
||||
} reg1 @ 0x0;
|
||||
|
||||
level2 inner2 @ 0x10;
|
||||
};
|
||||
|
||||
addrmap level0 {
|
||||
level1 inner1 @ 0x0;
|
||||
};
|
||||
"""
|
||||
return compile_rdl(rdl_source, top="level0")
|
||||
|
||||
|
||||
def test_external_nested_components_generate_correct_decoder(external_nested_rdl):
|
||||
"""Test that external nested components generate correct decoder logic.
|
||||
|
||||
The decoder should:
|
||||
- Generate select signals for multicast and port[16]
|
||||
- NOT generate select signals for multicast.common[] or multicast.response
|
||||
- NOT generate invalid paths like multicast.common[i0]
|
||||
"""
|
||||
with TemporaryDirectory() as tmpdir:
|
||||
exporter = BusDecoderExporter()
|
||||
exporter.export(
|
||||
external_nested_rdl,
|
||||
tmpdir,
|
||||
cpuif_cls=APB4Cpuif,
|
||||
)
|
||||
|
||||
# Read the generated module
|
||||
module_file = Path(tmpdir) / "buffer_t.sv"
|
||||
content = module_file.read_text()
|
||||
|
||||
# Should have correct select signals
|
||||
assert "cpuif_wr_sel.multicast = 1'b1;" in content
|
||||
assert "cpuif_wr_sel.port[i0] = 1'b1;" in content
|
||||
|
||||
# Should NOT have invalid nested paths
|
||||
assert "cpuif_wr_sel.multicast.common" not in content
|
||||
assert "cpuif_wr_sel.multicast.response" not in content
|
||||
assert "cpuif_rd_sel.multicast.common" not in content
|
||||
assert "cpuif_rd_sel.multicast.response" not in content
|
||||
|
||||
# Verify struct is flat (no nested structs for external children)
|
||||
assert "typedef struct packed" in content
|
||||
assert "logic multicast;" in content
|
||||
assert "logic [15:0]port;" in content
|
||||
|
||||
|
||||
def test_external_nested_components_generate_correct_interfaces(external_nested_rdl):
|
||||
"""Test that external nested components generate correct interface ports.
|
||||
|
||||
The module should have:
|
||||
- One master interface for multicast
|
||||
- Array of 16 master interfaces for port[]
|
||||
- NO interfaces for internal components like common[] or response
|
||||
"""
|
||||
with TemporaryDirectory() as tmpdir:
|
||||
exporter = BusDecoderExporter()
|
||||
exporter.export(
|
||||
external_nested_rdl,
|
||||
tmpdir,
|
||||
cpuif_cls=APB4Cpuif,
|
||||
)
|
||||
|
||||
# Read the generated module
|
||||
module_file = Path(tmpdir) / "buffer_t.sv"
|
||||
content = module_file.read_text()
|
||||
|
||||
# Should have master interfaces for top-level external children
|
||||
assert "m_apb_multicast" in content
|
||||
assert "m_apb_port [16]" in content or "m_apb_port[16]" in content
|
||||
|
||||
# Should NOT have interfaces for nested external children
|
||||
assert "m_apb_multicast_common" not in content
|
||||
assert "m_apb_multicast_response" not in content
|
||||
assert "m_apb_common" not in content
|
||||
assert "m_apb_response" not in content
|
||||
|
||||
|
||||
def test_non_external_nested_components_are_descended(compile_rdl):
|
||||
"""Test that non-external nested components are still descended into.
|
||||
|
||||
This is a regression test to ensure we didn't break normal nested
|
||||
component handling.
|
||||
"""
|
||||
rdl_source = """
|
||||
addrmap inner_block {
|
||||
reg {
|
||||
field {
|
||||
sw=rw;
|
||||
hw=r;
|
||||
} data[31:0];
|
||||
} inner_reg @ 0x0;
|
||||
};
|
||||
|
||||
addrmap outer_block {
|
||||
inner_block inner @ 0x0;
|
||||
};
|
||||
"""
|
||||
top = compile_rdl(rdl_source, top="outer_block")
|
||||
|
||||
with TemporaryDirectory() as tmpdir:
|
||||
exporter = BusDecoderExporter()
|
||||
exporter.export(top, tmpdir, cpuif_cls=APB4Cpuif)
|
||||
|
||||
# Read the generated module
|
||||
module_file = Path(tmpdir) / "outer_block.sv"
|
||||
content = module_file.read_text()
|
||||
|
||||
# Should descend into inner and reference inner_reg
|
||||
assert "inner" in content
|
||||
assert "inner_reg" in content
|
||||
|
||||
|
||||
def test_max_decode_depth_parameter_exists(compile_rdl):
|
||||
"""Test that max_decode_depth parameter can be set."""
|
||||
rdl_source = """
|
||||
addrmap simple {
|
||||
reg {
|
||||
field { sw=rw; hw=r; } data[31:0];
|
||||
} my_reg @ 0x0;
|
||||
};
|
||||
"""
|
||||
top = compile_rdl(rdl_source, top="simple")
|
||||
|
||||
with TemporaryDirectory() as tmpdir:
|
||||
exporter = BusDecoderExporter()
|
||||
# Should not raise an exception
|
||||
exporter.export(
|
||||
top,
|
||||
tmpdir,
|
||||
cpuif_cls=APB4Cpuif,
|
||||
max_decode_depth=2,
|
||||
)
|
||||
|
||||
# Verify output was generated
|
||||
module_file = Path(tmpdir) / "simple.sv"
|
||||
assert module_file.exists()
|
||||
|
||||
Reference in New Issue
Block a user