Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DETECTARRAY error on packed structures #610

Closed
veripoolbot opened this issue Feb 3, 2013 · 6 comments
Closed

DETECTARRAY error on packed structures #610

veripoolbot opened this issue Feb 3, 2013 · 6 comments
Labels
resolution: fixed Closed; fixed

Comments

@veripoolbot
Copy link
Contributor

Author Name: Jeremy Bennett (@jeremybennett)
Original Redmine Issue: 610 from https://www.veripool.org
Original Date: 2013-02-03
Original Assignee: Jeremy Bennett (@jeremybennett)

The following triggers the DETECTARRAY error (ID_MSB is a localparam with value 1):

    typedef struct packed {
       logic [ID_MSB:0] 	id;
    } context_t;

    context_t  tsb;

    assign tsb.id = {tsb.id[0], clk};

    initial begin
       tsb.id = 0;
    end

This is a classic loop within a vector, so triggers UNOPTFLAT. However if UNOPTFLAT is turned off (-Wno-UNOPTFLAT), we get:

%Error-DETECTARRAY: t/t_detectarray_1.v:21: Unsupported: Can't detect changes on complex variable (probably with UNOPTFLAT warning suppressed): v.tsb
%Error: Exiting due to 1 error(s)
%Error: See the manual and http://www.veripool.org/verilator for more assistance.

DETECTARRAY is not actually in the manual (I've fixed that). The problem is in @genChangeDet()@ in @V3Changed.cpp@, which can only deal with change on basic types and unpacked arrays of modest size.

I think this should not be too difficult to change for packed structures, since the standard allows them always to be treated as vectors (i.e. 1-D packed arrays). I'm working on a patch for this, but tripping over the type system a bit, so any hints on this are much appreciated.

While working on this I found a second bug. If in the above example, I don't use a localparam, but just an explict constant, 1, I trigger an internal error due to a broken link:

  typedef struct packed {
       logic [1:0] 	id;
    } context_t;

    context_t  tsb;

    assign tsb.id = {tsb.id[0], clk};

    initial begin
       tsb.id = 0;
    end

This triggers:

%Error: Internal Error: ../V3Broken.cpp:206: Broken link in node (or something without maybePointedTo)
%Error: Internal Error: See the manual and http://www.veripool.org/verilator for more assistance.

I haven't started looking at why this happens, but I suspect it is completely unrelated.

In the meantime, please pull a patch from branch detectarray at git@github.com:jeremybennett/verilator.git. This contains an update to the user guide, a test (t_detectarray_1) to demonstrate DETECTARRAY error with a packed structure and a test (t_detectarray_2) to trigger the broken link problem.
@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2013-02-03T18:35:45Z

Applied the doc change to git.

The git version deals with packed arrays as normal bit vectors so I suspect the test that prints the warning can be improved to just allow any AstPackArray or AstNodeClass (with isPacked).

It might be that the ChangeDet node later on then has another test that assumes a basic data type, if so that can be fixed similarly.

Please give it a try.

@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2013-02-03T19:13:22Z

The _2 broken error is fixed in git and the test now gets a DETECTARRAY error. If you want help with the DETECTARRAY let me know.

@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Jeremy Bennett (@jeremybennett)
Original Date: 2013-02-08T18:12:03Z

Took far longer than I intended. I made heavy weather of trying to break down the structure, before I realized it should just be treated as simple vector, so in the end it's a tiny patch.

I ran regression, which failed in two places. One is an apparent minor discrepancy between Verilator and SystemPerl headers which I have fixed (in Verilator). The other is t_vpi_var, which gets a mismatch (@rfr[39:0]=0x60064B1CDB@ versus @sig[39:0]=0x00064B1CDB@ in @arr[40]@). I don't know enough about VPI to see where that is going wrong. It also fails on the master branch, so I think it's not this patch. I'll raise this as a separate Issue.

Please pull a patch from branch detectarray at git@github.com:jeremybennett/verilator.git.

@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2013-02-10T14:54:54Z

Looks good. Pushed to git towards 3.846.

@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2013-02-10T17:33:50Z

P.S. The SystemPerl header issue is fixed by the new SystemPerl version I just pushed.

@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2013-03-09T21:51:27Z

In 3.846.

@veripoolbot veripoolbot added the resolution: fixed Closed; fixed label Dec 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
resolution: fixed Closed; fixed
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@veripoolbot