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

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

Comments

@veripoolbot
Copy link

@veripoolbot veripoolbot commented Feb 3, 2013


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

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Feb 3, 2013


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

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Feb 3, 2013


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

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Feb 8, 2013


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

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Feb 10, 2013


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


Looks good. Pushed to git towards 3.846.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Feb 10, 2013


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

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Mar 9, 2013


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


In 3.846.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
1 participant
You can’t perform that action at this time.