Simplify top generator.

[were]

No description provided.

[Copilot]

Pull Request Overview

This PR refactors the top-level Verilog harness generation to improve code organization and testability. The refactoring introduces structured dataclasses to replace dictionary-based data passing, extracts shared predicate utilities into a dedicated module, and creates focused helper functions for different aspects of top-level generation.

Key Changes:

• Introduced TopHarnessBuilder class to encapsulate harness code emission with explicit state management
• Extracted predicate reduction and mux-chain logic into a shared predicates.py module for reuse across cleanup and top-level generation
• Replaced dictionary returns with typed dataclasses (SRAMInfo, SRAMParams, TestbenchTemplateConfig, ResourceCopyPlan) to improve type safety and IDE support

Reviewed Changes

Copilot reviewed 25 out of 25 changed files in this pull request and generated 16 comments.

Show a summary per file

File	Description
python/unit-tests/codegen/test_top_harness_sections.py	New unit tests for refactored top harness helper functions
python/unit-tests/codegen/test_external_exposure_predicate.py	Tests verifying external exposure bookkeeping preserves predicate metadata
python/unit-tests/codegen/test_cleanup.py	Updated imports to use new predicates module
python/ci-tests/test_select1hot.py	Added test for two-value select1hot case, plus design emission helper
python/assassyn/codegen/verilog/utils.py	Refactored SRAM info extraction to return dataclasses instead of dicts
python/assassyn/codegen/verilog/top.py	Introduced TopHarnessBuilder and extracted helpers for SRAM, FIFO, and trigger counter emission
python/assassyn/codegen/verilog/testbench.py	Refactored to use TestbenchTemplateConfig dataclass for structured template parameters
python/assassyn/codegen/verilog/elaborate.py	Introduced ResourceCopyPlan and related dataclasses to make file copying deterministic and testable
python/assassyn/codegen/verilog/predicates.py	New module containing shared predicate utilities (reduce_predicates, emit_predicate_mux_chain)
python/assassyn/codegen/verilog/cleanup.py	Updated to use shared predicate utilities, replaced dict access with dataclass field access
python/assassyn/codegen/verilog/module.py	Updated to use SRAMInfo dataclass fields instead of dictionary access
python/assassyn/codegen/verilog/system.py	Removed obsolete external wire tracking lists
python/assassyn/codegen/verilog/design.py	Removed obsolete predicate helper, updated to use SRAMParams dataclass
python/assassyn/codegen/verilog/_expr/intrinsics.py	Simplified external output handling, stores raw meta_cond instead of formatted condition
python/assassyn/codegen/verilog/_expr/arith.py	Fixed select1hot two-value case to emit assignment consistently
python/assassyn/builder/type_oriented_namer.py	Updated operator symbols to use trailing underscore convention (and_, or_, xor_)
*.md files	Updated documentation to reflect new module structure and dataclass usage

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

[nitpick] The test at line 43 filters lines using "fifo_producer_" in line.lower(), which converts the line to lowercase for the check. However, it then checks if lines[0].startswith("fifo_Producer_outA_push_valid") with the original casing. This could fail if the actual output has different casing than expected.

The filter converts to lowercase, so it would match "fifo_Producer_" or "fifo_producer_", but the assertion checks for the exact case "fifo_Producer_". This is likely correct if the output maintains the original module name casing, but consider making the test more explicit about case sensitivity or use consistent casing in both the filter and assertion.

[Copilot]

The type annotation str | None uses Python 3.10+ union syntax, but the file imports from __future__ import annotations which enables PEP 563 (postponed evaluation of annotations). While this makes the syntax work at parse time, it may cause issues with runtime type checkers or the @enforce_type decorator if it evaluates annotations at runtime.

Consider using Optional[str] from typing for compatibility with the @enforce_type decorator:

from typing import Callable, Optional, Sequence, Tuple, TypeVar

def reduce_predicates(
    predicates: Sequence[str],
    *,
    default_literal: Optional[str],
    op: str = "or_",
) -> str:

This ensures compatibility with runtime type checking used by @enforce_type.

[Copilot]

The _resolve_alias_resources function sorts matches at line 103 with matches = sorted(set(re.findall(pattern, top_content))), but the sorting is done after extracting unique suffixes. This is good for deterministic output. However, the sorting uses string comparison which may not give numerically correct order (e.g., "10" would come before "2").

Consider sorting numerically:

matches = sorted(set(re.findall(pattern, top_content)), key=int)

This ensures that alias modules are processed in the correct numeric order.

Suggested change

	matches = sorted(set(re.findall(pattern, top_content)))
	matches = sorted(set(re.findall(pattern, top_content)), key=int)

[Copilot]

[nitpick] The indent_block context manager modifies self.indent in a try-finally block, but if an exception occurs during the yield, the indentation will still be restored. This is correct behavior. However, the docstring should clarify that the indentation change is temporary and will be restored even if an exception occurs within the context:

@contextmanager
def indent_block(self, delta: int = 4):
    """Context manager that temporarily increases indentation.
    
    The indentation change is automatically reverted when exiting the context,
    even if an exception occurs.
    """

[Copilot]

[nitpick] The documentation references predicates.reduce_predicates and predicates.emit_predicate_mux_chain without the full module path. For consistency with the rest of the documentation and clarity, these should reference the full module path: python.assassyn.codegen.verilog.predicates.reduce_predicates and python.assassyn.codegen.verilog.predicates.emit_predicate_mux_chain.

[Copilot]

The two-value case now returns an assignment expression directly, which should be consistent with the multi-value case that also appends code. However, this creates an inconsistency: when num_values == 2, the function returns a string (to be assigned), while for other cases it returns None after appending code. This mixed return behavior could be confusing.

Consider making the two-value case consistent by always appending code and returning None:

dumper.append_code(f"{rval} = Mux({cond}.as_bits()[1], {values[0]}, {values[1]})")
return None

Suggested change

	return f"{rval} = Mux({cond}.as_bits()[1], {values[0]}, {values[1]})"
	dumper.append_code(f"{cond}_res = Bits({selector_bits})(0)")
	dumper.append_code(f"{rval} = Mux({cond}.as_bits()[1], {values[0]}, {values[1]})")
	return None

[Copilot]

[nitpick] The variable name init_path is misleading when init_file is None. At line 211, init_path is set to None but then the code checks if init_path is not None at line 242. Consider renaming the variable to resolved_init_path to better reflect that it's the resolved path (or None if no init file exists):

resolved_init_path = None
if init_file:
    resolved_init_path = Path(init_file)
    if init_root is not None and not resolved_init_path.is_absolute():
        resolved_init_path = init_root / resolved_init_path

[Copilot]

[nitpick] In the SRAM address assignment fallback case, the code now correctly handles single-element arrays by ensuring index_bits is at least 1:

index_bits = array.index_bits if array.index_bits > 0 else 1

However, this pattern is already used in other parts of the codebase (e.g., line 48 in module.py). The logic is correct, but it's duplicated. Consider extracting this into a helper function in utils.py to ensure consistent handling across the codebase.

[Copilot]

Import pollutes the enclosing namespace, as the imported module assassyn.frontend does not define 'all'.

Suggested change

	from assassyn.frontend import * # type: ignore
	from assassyn.frontend import SysBuilder, Module, module, RegArray, Int, UInt, Port, log # type: ignore

[Copilot]

Import of 'pytest' is not used.

Suggested change

import pytest