Fix nonconstant index errors in Questa by using unpacked structs and interface array intermediates (#17)
* Initial plan * Fix nonconstant index error by using unpacked structs and arrays Co-authored-by: arnavsacheti <36746504+arnavsacheti@users.noreply.github.com> * Add comprehensive tests for Questa compatibility with instance arrays Co-authored-by: arnavsacheti <36746504+arnavsacheti@users.noreply.github.com> * Add intermediate signals for interface array fanin to fix Questa compatibility Questa simulator rejects indexing interface arrays with non-constant variables in procedural blocks (always_comb). This fix creates intermediate unpacked array signals that are assigned from interface arrays using generate loops (with genvar), then references those intermediates in the fanin logic. Changes: - Added fanin_intermediate_gen.py to generate intermediate signals - Modified APB3/APB4/AXI4-Lite cpuif classes to use intermediates for interface arrays - Updated templates to include intermediate signal section - Intermediate signals use genvar indexing (legal) instead of variable indexing (illegal in Questa) Co-authored-by: arnavsacheti <36746504+arnavsacheti@users.noreply.github.com> * Fix type checking and formatting issues in fanin intermediate generator - Added proper null checks for node.array_dimensions to avoid type errors - Used getattr() to safely access is_interface and _interface attributes - Added early returns when interface is None to prevent errors - Fixed formatting issues in fanin_intermediate_gen.py - All type checks now pass (only pre-existing errors remain in other files) - All 67 tests still pass Co-authored-by: arnavsacheti <36746504+arnavsacheti@users.noreply.github.com> * Fix test collection when cocotb is not installed The cocotb test files (test_runner.py) have imports that fail at module level when cocotb is not installed, causing pytest collection to fail. This prevents running tests in environments without cocotb dependencies. Solution: Updated conftest.py to conditionally ignore cocotb tests when cocotb is not available, allowing tests to pass in both environments (with and without cocotb). - When cocotb is available: only ignore test_register_access.py files (existing behavior) - When cocotb is not available: ignore all files under cocotb/** directories This allows CI to run all 67 non-simulation tests successfully without requiring cocotb/verilator dependencies for basic testing. Co-authored-by: arnavsacheti <36746504+arnavsacheti@users.noreply.github.com> * add default for unpacked struct * cocotb is test dependancy * Refactor fanin intermediate signal logic to cpuif level Moved the interface-specific signal assignment logic from fanin_intermediate_gen.py to individual cpuif classes (APB3Cpuif, APB4Cpuif, AXI4LiteCpuif). This follows better architecture principles where each cpuif knows which signals it needs. Changes: - Added fanin_intermediate_assignments() method to BaseCpuif - Implemented fanin_intermediate_assignments() in APB3Cpuif, APB4Cpuif, and AXI4LiteCpuif - Updated FaninIntermediateGenerator to call the cpuif method instead of checking interface type - Removed interface type checking logic from fanin_intermediate_gen.py This makes the code more maintainable and follows the single responsibility principle - each cpuif class knows its own signal requirements. 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:
Copilot
142
src/peakrdl_busdecoder/cpuif/fanin_intermediate_gen.py
Normal file
142
src/peakrdl_busdecoder/cpuif/fanin_intermediate_gen.py
Normal file
@@ -0,0 +1,142 @@
|
||||
"""Generator for intermediate signals needed for interface array fanin.
|
||||
|
||||
When using SystemVerilog interface arrays, we cannot use variable indices
|
||||
in procedural blocks (like always_comb). This generator creates intermediate
|
||||
signals that copy from interface arrays using generate loops, which can then
|
||||
be safely accessed with variable indices in the fanin logic.
|
||||
"""
|
||||
|
||||
from collections import deque
|
||||
from typing import TYPE_CHECKING
|
||||
|
||||
from systemrdl.node import AddressableNode
|
||||
from systemrdl.walker import WalkerAction
|
||||
|
||||
from ..body import Body, ForLoopBody
|
||||
from ..design_state import DesignState
|
||||
from ..listener import BusDecoderListener
|
||||
from ..utils import get_indexed_path
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from .base_cpuif import BaseCpuif
|
||||
|
||||
|
||||
class FaninIntermediateGenerator(BusDecoderListener):
|
||||
"""Generates intermediate signals for interface array fanin."""
|
||||
|
||||
def __init__(self, ds: DesignState, cpuif: "BaseCpuif") -> None:
|
||||
super().__init__(ds)
|
||||
self._cpuif = cpuif
|
||||
self._declarations: list[str] = []
|
||||
self._stack: deque[Body] = deque()
|
||||
self._stack.append(Body())
|
||||
|
||||
def enter_AddressableComponent(self, node: AddressableNode) -> WalkerAction | None:
|
||||
action = super().enter_AddressableComponent(node)
|
||||
|
||||
# Only generate intermediates for interface arrays
|
||||
# Check if cpuif has is_interface attribute (some implementations don't)
|
||||
is_interface = getattr(self._cpuif, "is_interface", False)
|
||||
if not is_interface or not node.array_dimensions:
|
||||
return action
|
||||
|
||||
# Generate intermediate signal declarations
|
||||
self._generate_intermediate_declarations(node)
|
||||
|
||||
# Generate assignment logic using generate loops
|
||||
if node.array_dimensions:
|
||||
for i, dim in enumerate(node.array_dimensions):
|
||||
fb = ForLoopBody(
|
||||
"genvar",
|
||||
f"gi{i}",
|
||||
dim,
|
||||
)
|
||||
self._stack.append(fb)
|
||||
|
||||
# Generate assignments from interface array to intermediates
|
||||
self._stack[-1] += self._generate_intermediate_assignments(node)
|
||||
|
||||
return action
|
||||
|
||||
def exit_AddressableComponent(self, node: AddressableNode) -> None:
|
||||
is_interface = getattr(self._cpuif, "is_interface", False)
|
||||
if is_interface and node.array_dimensions:
|
||||
for _ in node.array_dimensions:
|
||||
b = self._stack.pop()
|
||||
if not b:
|
||||
continue
|
||||
self._stack[-1] += b
|
||||
|
||||
super().exit_AddressableComponent(node)
|
||||
|
||||
def _generate_intermediate_declarations(self, node: AddressableNode) -> None:
|
||||
"""Generate intermediate signal declarations for a node."""
|
||||
inst_name = node.inst_name
|
||||
|
||||
# Array dimensions should be checked before calling this function
|
||||
if not node.array_dimensions:
|
||||
return
|
||||
|
||||
# Calculate total array size
|
||||
array_size = 1
|
||||
for dim in node.array_dimensions:
|
||||
array_size *= dim
|
||||
|
||||
# Create array dimension string
|
||||
array_str = "".join(f"[{dim}]" for dim in node.array_dimensions)
|
||||
|
||||
# Generate declarations for each fanin signal
|
||||
# For APB3/4: PREADY, PSLVERR, PRDATA
|
||||
# These are the signals read in fanin
|
||||
self._declarations.append(f"logic {inst_name}_fanin_ready{array_str};")
|
||||
self._declarations.append(f"logic {inst_name}_fanin_err{array_str};")
|
||||
self._declarations.append(
|
||||
f"logic [{self._cpuif.data_width - 1}:0] {inst_name}_fanin_data{array_str};"
|
||||
)
|
||||
|
||||
def _generate_intermediate_assignments(self, node: AddressableNode) -> str:
|
||||
"""Generate assignments from interface array to intermediate signals."""
|
||||
inst_name = node.inst_name
|
||||
indexed_path = get_indexed_path(node.parent, node, "gi", skip_kw_filter=True)
|
||||
|
||||
# Get master prefix - use getattr to avoid type errors
|
||||
interface = getattr(self._cpuif, "_interface", None)
|
||||
if interface is None:
|
||||
return ""
|
||||
master_prefix = interface.get_master_prefix()
|
||||
|
||||
# Array dimensions should be checked before calling this function
|
||||
if not node.array_dimensions:
|
||||
return ""
|
||||
|
||||
# Create indexed signal names for left-hand side
|
||||
array_idx = "".join(f"[gi{i}]" for i in range(len(node.array_dimensions)))
|
||||
|
||||
# Delegate to cpuif to get the appropriate assignments for this interface type
|
||||
assignments = self._cpuif.fanin_intermediate_assignments(
|
||||
node, inst_name, array_idx, master_prefix, indexed_path
|
||||
)
|
||||
|
||||
return "\n".join(assignments)
|
||||
|
||||
def get_declarations(self) -> str:
|
||||
"""Get all intermediate signal declarations."""
|
||||
if not self._declarations:
|
||||
return ""
|
||||
return "\n".join(self._declarations)
|
||||
|
||||
def __str__(self) -> str:
|
||||
"""Get all intermediate signal declarations and assignments."""
|
||||
if not self._declarations:
|
||||
return ""
|
||||
|
||||
# Output declarations first
|
||||
output = "\n".join(self._declarations)
|
||||
output += "\n\n"
|
||||
|
||||
# Then output assignments
|
||||
body_str = "\n".join(map(str, self._stack))
|
||||
if body_str and body_str.strip():
|
||||
output += body_str
|
||||
|
||||
return output
|
||||
Reference in New Issue
Block a user