Added safety guards to la_asynfifo depth parameter#173
Added safety guards to la_asynfifo depth parameter#173RiceShelley wants to merge 1 commit intomainfrom
Conversation
WalkthroughVersion bump to 0.12.1 with simplified asyncfifo logic by removing DEPTH==1 special case handling and enforcing DEPTH≥2 and power-of-two constraints via runtime checks. Test depths adjusted to match new constraints. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
This PR adds parameter validation to the la_asyncfifo RTL to restrict DEPTH to valid configurations (power-of-2, > 1), and updates tests/versioning accordingly.
Changes:
- Add an
initialparameter check inla_asyncfifo.vto error/finish whenDEPTHis invalid. - Simplify
wr_almost_full_nextlogic by removing theDEPTH==1generate special-case and standardizing address width computation. - Update the async FIFO cocotb test matrix to only use valid depths, and bump the Python package version.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/ramlib/test_la_asyncfifo.py | Narrows tested FIFO depths to power-of-2 values consistent with the new RTL restriction. |
| lambdalib/ramlib/la_asyncfifo/rtl/la_asyncfifo.v | Enforces DEPTH validity via an initial check and removes now-unneeded DEPTH==1 special-casing. |
| lambdalib/init.py | Bumps package version to 0.12.1. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @pytest.mark.parametrize("depth, simulator, output_wave", list(itertools.product( | ||
| [1, 2, 3, 4, 5, 6, 7, 8, 9], | ||
| [2, 4, 8, 16], | ||
| ["icarus", "verilator"], | ||
| [False] |
There was a problem hiding this comment.
With the RTL now enforcing DEPTH to be a power of 2 and > 1, the test matrix no longer exercises the new guard behavior (e.g., DEPTH=1 or non-power-of-2 values). Consider adding a small negative test case that asserts simulation/build fails for an invalid depth, so the safety check is covered and doesn't regress silently.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/ramlib/test_la_asyncfifo.py (1)
7-11: Please exercise the guard path explicitly.This matrix now only covers valid
DEPTHvalues, so the new rejection logic has no regression coverage. Keep a negative case forDEPTH=1, and if the power-of-two restriction stays, one for a non-power-of-two depth as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/ramlib/test_la_asyncfifo.py` around lines 7 - 11, The parameter matrix in the pytest.mark.parametrize call only exercises valid DEPTH values so the new rejection logic isn't covered; update the param list in tests/ramlib/test_la_asyncfifo.py's pytest.mark.parametrize (the depth parameter) to include a negative case DEPTH=1 and a non-power-of-two value (e.g., 3) alongside the existing [2,4,8,16], and ensure the test asserts the rejection path for those values (either mark expected failure or assert the constructor/initialization raises/returns the proper error) so both guard branches are exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lambdalib/ramlib/la_asyncfifo/rtl/la_asyncfifo.v`:
- Line 24: Remove the new restriction that forces parameter DEPTH to be a
power-of-two: revert the change to the DEPTH declaration/comment and delete the
added validation/assert that rejects non-power-of-two values (the check
implemented near the DEPTH parameter and the subsequent assert/validation block
around the address-width/calculation). Preserve the existing free-running
synchronized counter/address-path logic already in la_asyncfifo (the reason the
module supports non-power-of-two sizes), restore the original comment that
allowed arbitrary DEPTH > 1, and ensure tests exercising non-power-of-two depths
(e.g., 3, 5, 6) will continue to pass.
- Line 50: The code computes localparam AW = $clog2(DEPTH) which becomes 0 for
DEPTH==1 and leads to illegal elaboration-time expressions like [AW-1:0] and
{{(AW-1){...}}} used in signal declarations and replications; fix this by making
AW and the dependent declarations elaboration-safe: either set AW = (DEPTH>1) ?
$clog2(DEPTH) : 1 so widths never go negative, or wrap the declarations and
replications that use AW (the vector slices and the replication constructs) in a
generate block conditional on (DEPTH>1) and provide an explicit alternate
(simple scalar/special-case) implementation for DEPTH==1; update all places that
reference AW, DEPTH, and the problematic vector/replication constructs so the
negative-width case cannot be elaborated.
---
Nitpick comments:
In `@tests/ramlib/test_la_asyncfifo.py`:
- Around line 7-11: The parameter matrix in the pytest.mark.parametrize call
only exercises valid DEPTH values so the new rejection logic isn't covered;
update the param list in tests/ramlib/test_la_asyncfifo.py's
pytest.mark.parametrize (the depth parameter) to include a negative case DEPTH=1
and a non-power-of-two value (e.g., 3) alongside the existing [2,4,8,16], and
ensure the test asserts the rejection path for those values (either mark
expected failure or assert the constructor/initialization raises/returns the
proper error) so both guard branches are exercised.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: edca5e6a-305c-431d-a02a-13882e461c17
📒 Files selected for processing (3)
lambdalib/__init__.pylambdalib/ramlib/la_asyncfifo/rtl/la_asyncfifo.vtests/ramlib/test_la_asyncfifo.py
|
|
||
| module la_asyncfifo #(parameter DW = 32, // Memory width | ||
| parameter DEPTH = 4, // FIFO depth | ||
| parameter DEPTH = 4, // FIFO depth (must be power of 2 and greater than 1) |
There was a problem hiding this comment.
Please don't narrow DEPTH to powers of two unless that API break is intentional.
The FIFO still keeps a free-running synchronized counter separate from the wrapped RAM address pointer, and the note at Lines 95-98 says that path was added to support non-power-of-two depths. This check now rejects values like 3, 5, and 6, while tests/ramlib/test_la_asyncfifo.py, Lines 7-11 were trimmed to stop exercising them. That looks like a backward-compatibility regression rather than a safety fix.
Also applies to: 183-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lambdalib/ramlib/la_asyncfifo/rtl/la_asyncfifo.v` at line 24, Remove the new
restriction that forces parameter DEPTH to be a power-of-two: revert the change
to the DEPTH declaration/comment and delete the added validation/assert that
rejects non-power-of-two values (the check implemented near the DEPTH parameter
and the subsequent assert/validation block around the
address-width/calculation). Preserve the existing free-running synchronized
counter/address-path logic already in la_asyncfifo (the reason the module
supports non-power-of-two sizes), restore the original comment that allowed
arbitrary DEPTH > 1, and ensure tests exercising non-power-of-two depths (e.g.,
3, 5, 6) will continue to pass.
|
|
||
| // local params | ||
| localparam AW = (DEPTH == 1) ? 1 : $clog2(DEPTH); | ||
| localparam AW = $clog2(DEPTH); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Verilog/SystemVerilog, if localparam AW = $clog2(DEPTH)andDEPTH=1soAW=0, are expressions like [AW-1:0]and{{(AW-1){1'b0}}, 1'b1}legal during elaboration, or can they fail before aninitial begin $error; $finish; endguard runs? Also, do synthesis tools generally enforceinitial-block parameter guards?
💡 Result:
In Verilog/SystemVerilog, when localparam AW = $clog2(DEPTH) with DEPTH=1 results in AW=0, expressions like [AW-1:0] (i.e., [-1:0]) and {{(AW-1){1'b0}}, 1'b1} (i.e., {{-1{1'b0}}, 1'b1}) are illegal during elaboration and will typically cause simulators and synthesis tools to fail with errors before any initial block executes. For [AW-1:0]: - This declares a packed array with invalid range [-1:0]. SystemVerilog requires packed array ranges to have positive width (left > right for descending ranges). Tools like Vivado report "single value range is not allowed in packed dimension" or similar syntax/elaboration errors for zero/negative widths. Verilator and Yosys assert or error on zero-width signals (e.g., "chunk.width >= 0" failure, replication value 0 only legal under concatenation). For {{(AW-1){1'b0}}, 1'b1}: - The replication operator {n{value}} with n=-1 is invalid (n must be non-negative constant). LRM (IEEE 1800-2017 §11.4.12.1) explicitly allows n=0 (ignored, size 0), but not negative. Tools error on non-positive replication multipliers. These fail at parse/elaboration time, before simulation/synthesis proceeds to initial blocks, so guards like initial begin if (DEPTH==1) $error; $finish; end do not run. Synthesis tools (Vivado, Yosys, Verilator, Synopsys) do NOT generally execute or enforce initial-block parameter guards. Synthesis ignores or elaborates initial blocks without running procedural code (no simulation semantics). Parameter validation must use generate/if on parameters or tool-specific attributes; initial checks are simulation-only.
Citations:
- 1: https://stackoverflow.com/questions/39444335/is-clog2-task-supported-in-verilog-hdl
- 2: https://www.reddit.com/r/Verilog/comments/1c76sia/input_declaration_using_clog2/
- 3: https://electronics.stackexchange.com/questions/555457/single-value-range-is-not-allowed-in-packed-dimension
- 4: Zero-width signals issue in combination with Verilator zachjs/sv2v#163
- 5: Being able to specify a zero width in an indexed part-select operation violates LRM verilator/verilator#5028
- 6: Valid RTLIL produces invalid Verilog, which Yosys then cannot read back YosysHQ/yosys#948
- 7: https://stackoverflow.com/questions/27837882/replication-operator-with-0
- 8: ERROR: Assert `chunk.width >= 0' failed in kernel/rtlil.cc:3039. YosysHQ/yosys#129
- 9: https://www.veripool.org/ftp/verilator_doc.pdf
- 10: Incorrect output when
$clogused in nested function calls YosysHQ/yosys#3618 - 11: "Memory with non-constant width or size!" message, when a function which returns a constant value is used YosysHQ/yosys#1799
- 12: https://www.xilinx.com/content/dam/xilinx/support/documents/sw_manuals/xilinx2021_2/ug901-vivado-synthesis.pdf
🏁 Script executed:
find . -name "la_asyncfifo.v" -type fRepository: siliconcompiler/lambdalib
Length of output: 119
🏁 Script executed:
wc -l ./lambdalib/ramlib/la_asyncfifo/rtl/la_asyncfifo.vRepository: siliconcompiler/lambdalib
Length of output: 123
🏁 Script executed:
cat -n ./lambdalib/ramlib/la_asyncfifo/rtl/la_asyncfifo.vRepository: siliconcompiler/lambdalib
Length of output: 8451
Initial block parameter guard at lines 183–189 does not protect against elaboration-time errors.
With DEPTH=1, AW=0, making expressions like [AW-1:0] (i.e., [-1:0]) and {{(AW-1){1'b0}}, ...} (i.e., {{-1{...}}) illegal at elaboration time. Lines 89, 147, 175, and 178 use these invalid expressions and will fail elaboration before the initial block ever executes. Additionally, synthesis tools ignore initial blocks entirely and do not enforce procedural guards. Use elaboration-time validation (e.g., generate blocks on parameters) or tool-specific constraints instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lambdalib/ramlib/la_asyncfifo/rtl/la_asyncfifo.v` at line 50, The code
computes localparam AW = $clog2(DEPTH) which becomes 0 for DEPTH==1 and leads to
illegal elaboration-time expressions like [AW-1:0] and {{(AW-1){...}}} used in
signal declarations and replications; fix this by making AW and the dependent
declarations elaboration-safe: either set AW = (DEPTH>1) ? $clog2(DEPTH) : 1 so
widths never go negative, or wrap the declarations and replications that use AW
(the vector slices and the replication constructs) in a generate block
conditional on (DEPTH>1) and provide an explicit alternate (simple
scalar/special-case) implementation for DEPTH==1; update all places that
reference AW, DEPTH, and the problematic vector/replication constructs so the
negative-width case cannot be elaborated.
No description provided.