Skip to content

Fix #1317: Variable written in an always_ff should not driven by other process#1374

Open
MrCookieeeee wants to merge 1 commit into
steveicarus:masterfrom
MrCookieeeee:fix-1317
Open

Fix #1317: Variable written in an always_ff should not driven by other process#1374
MrCookieeeee wants to merge 1 commit into
steveicarus:masterfrom
MrCookieeeee:fix-1317

Conversation

@MrCookieeeee
Copy link
Copy Markdown
Contributor

This PR fix #1371

This PR adds a check for variables written by always_ff processes, following the SystemVerilog single-writer rule for always_ff.

Implementation

  1. First, it scans all always_ff processes and collects the Nexus objects written by each process through nex_output(). A map is used to record which always_ff process first writes each Nexus. If another always_ff process writes the same Nexus, an error is reported.

  2. Second, it scans the remaining non-always_ff processes and checks whether any of them also write a Nexus that was already written by an always_ff process. If so, an error is reported. Compiler-scheduled initialization processes marked with _ivl_schedule_init are skipped, so declaration initializers are not treated as conflicting procedural writers.

Overhead

For time complexity, this check walks the process list twice and calls nex_output() once for each relevant process. The map/set lookups are logarithmic, so the cost is roughly proportional to the total number of procedural write targets, with log(N) overhead from the containers. I think this should be acceptable for normal elaboration-time checking, but I would appreciate feedback if there is a better or more idiomatic way to do this in Icarus.

Discussion

One limitation is that this check currently works at the Nexus level and does not explicitly distinguish disjoint bit/part-select writes. For example:

always_ff @(posedge clk)
    q[3:0] <= a;

always_ff @(posedge clk)
    q[7:4] <= b;

I did not add special handling for this case yet. Please let me know whether this is necessary. If so, I could add some check on it. :D

Comment thread ivtest/regress-sv.list Outdated
Comment thread elaborate.cc Outdated
Comment thread elaborate.cc Outdated
@MrCookieeeee
Copy link
Copy Markdown
Contributor Author

Hi,

I have updated the code and resubmitted the PR.

The changes are:

  1. In elaborate.cc, I removed the redundant if (outputs.size() == 0) check and changed the iterator declaration to use auto cur to keep the line shorter.

  2. I added the new regression test always_ff_lval_fail to regress-vvp.list, together with the corresponding always_ff_lval_fail.json and always_ff_lval_fail-iverilog-stderr.gold files.

Thanks.

Comment thread ivtest/regress-sv.list Outdated
@MrCookieeeee
Copy link
Copy Markdown
Contributor Author

Copy, i will fix it real quick.

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