Repository navigation
fix(sv): count writes through called subroutines as process drivers - #1135
Conversation
Variable driver validation only looked at assignment targets, so a variable written through a call's output argument, or by a called subroutine body, could also be written by another process. The driver scan now follows each call to its output actuals and its body, which write for the calling process (IEEE 1800-2023 9.2.2.2).
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 310dd14579
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| driver_id, | ||
| net_lvalue_range(lvalue, constants, parameter_types), | ||
| )); | ||
| for (param, arg) in subroutine.params.iter().zip(&args) { |
There was a problem hiding this comment.
When a call omits an input with a default expression, zipping the parameters only with the supplied args skips that default entirely. For example, if f(input logic a = side_effect()) is invoked as f() and side_effect() assigns module variable q, this scan does not attribute the write to the calling process, so a second always_ff that writes q is incorrectly accepted. The FF target scan has the same omission, even though call lowering evaluates param.default; traverse the selected defaults for omitted arguments and their nested calls.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Reproduced and fixed in eb514c1: the driver scan and the FF target scan now also follow the calls in the defaults of omitted arguments. Covered by review_regressions::rejects_variables_also_written_by_default_argument_calls.
| if visited.insert(name) { | ||
| self.body(drivers, &subroutine.body, driver_id, visited); |
There was a problem hiding this comment.
With a sufficiently long acyclic chain of subroutine calls, this newly recursive scan descends once per callee without enforcing the existing MAX_CALL_DEPTH limit of 64. Because driver validation runs before the comb/FF lowerers perform their guarded call traversal, a generated or otherwise large input can exhaust the compiler thread's stack instead of returning the intended subroutine call depth limit exceeded diagnostic. Track depth here or make the call-graph walk iterative.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in eb514c1: the driver scan carries the call depth and stops beyond MAX_CALL_DEPTH; process lowering then reports the depth limit as before.
Omitted arguments evaluate their defaults, whose calls may write variables for the calling process; the driver and FF target scans now follow them. The driver scan stops at the call depth limit, which the process lowering reports.
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
…rivers Follow calls in the select positions of assignment targets, which master now runs, when scanning for drivers.
1 participant
Summary
Validation of variable drivers only looked at assignment targets. A variable written another way could therefore also be written by a second process without any error:
always_ff ... x <= pick(q, q);next toalways_ff ... q <= d;qA subroutine writes for the process that calls it (IEEE 1800-2023 9.2.2.2), so these designs are now rejected with "multiple variable drivers", like other conflicting drivers. The driver scan follows each call it finds in a statement or expression to its output and inout actuals, and then into the subroutine body, recursively and visiting each subroutine once. A variable written only through one process's calls is still accepted.
collect_callsmoves from the FF lowering to the shared procedural helpers. A newstmt_callsgathers the calls in one statement's own expressions, now including system task arguments and repeat counts. The FF commit-target scan usesstmt_callstoo.Tests
SystemVerilog-only regressions, since Verilator and Icarus do not reject multiply driven variables:
review_regressions::rejects_variables_also_written_through_output_argumentsreview_regressions::rejects_variables_also_written_by_called_subroutine_bodiesreview_regressions::accepts_a_variable_written_only_through_one_process_callsValidation
cargo test -p celox --features systemverilog --test systemverilog: 1431 passed.::svtests with--include-ignored: 837 passed / 91 failed, the same failing set as master.cargo test -p celox-test-suite --features verilator,icaruspasses, including the retained-report checks.conformance/sv-tests/expected.tsv.