Skip to content

ci: RTL testbenches and zero-multiplier check in CI (M4 Task 7 step 2) - #21

Merged
jackthepunished merged 1 commit into
mainfrom
m4-ci
Aug 8, 2026
Merged

jackthepunished merged 1 commit into
mainfrom
m4-ci

Conversation

@jackthepunished

@jackthepunished jackthepunished commented Aug 8, 2026

Copy link
Copy Markdown
Owner

Per docs/plans/2026-08-07-phase4-m4-rtl.md Task 7 step 2: CI now installs verilator+yosys and runs make lint rtl_test synth_check after the existing make test. rtl_test includes the export64 case (needs the cpu torch already installed for pytest); nothing depends on data/v1. Expect the job to lengthen by roughly the synth_xilinx runtime. Merging only after this PR's own workflow run is green.

Summary by CodeRabbit

  • Chores
    • Enhanced automated validation by installing additional hardware design tools.
    • Added linting, RTL tests, and synthesis checks to the continuous integration workflow.

Copilot AI lite review requested due to automatic review settings August 8, 2026 12:32

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@coderabbitai

coderabbitai Bot commented Aug 8, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The CI test job now installs Verilator and Yosys. It runs linting, RTL tests, and synthesis checks after the existing Python test suite.

Changes

CI validation

Layer / File(s) Summary
RTL checks in CI
.github/workflows/ci.yml
The test job installs Verilator and Yosys. It runs make lint rtl_test and make synth_check after the Python tests.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the CI changes, including RTL tests and the zero-multiplier synthesis check.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch m4-ci

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/ci.yml:
- Line 15: Update the Makefile:synth_check recipe invoked by the CI run to
ensure Yosys failures propagate through its tee pipeline, either by enabling
pipefail for the recipe shell or by removing the pipeline; keep synth_check
failing whenever Yosys fails.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: ab563f6c-b898-48ca-9456-d60de3ce9c08

📥 Commits

Reviewing files that changed from the base of the PR and between 0cfc32b and 4b0b387.

📒 Files selected for processing (1)
  • .github/workflows/ci.yml

Comment thread .github/workflows/ci.yml
- run: .venv/bin/pip install torch --index-url https://download.pytorch.org/whl/cpu --quiet
- run: make test
- run: make lint rtl_test
- run: make synth_check

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Make synth_check propagate Yosys failures.

Line 15 relies on Makefile:synth_check, which runs Yosys through tee without enabling pipefail. In shells without pipefail, the pipeline status is the last command’s status, so a successful tee can hide a failed Yosys process. (pubs.opengroup.org)

Update synth_check to use a shell with pipefail, or avoid the pipeline, before relying on this step as a synthesis gate.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/ci.yml at line 15, Update the Makefile:synth_check recipe
invoked by the CI run to ensure Yosys failures propagate through its tee
pipeline, either by enabling pipefail for the recipe shell or by removing the
pipeline; keep synth_check failing whenever Yosys fails.

@jackthepunished
jackthepunished merged commit 9328211 into main Aug 8, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants