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

Don't warn about little endian ordering for arrays of cells #1202

Closed
veripoolbot opened this issue Sep 11, 2017 · 6 comments
Closed

Don't warn about little endian ordering for arrays of cells #1202

veripoolbot opened this issue Sep 11, 2017 · 6 comments

Comments

@veripoolbot
Copy link

@veripoolbot veripoolbot commented Sep 11, 2017


Author Name: Mike Popoloski
Original Redmine Issue: 1202 from https://www.veripool.org


If you declare an array of instances (like say, interfaces) you will pretty much always get a warning about how it's in little endian order even though that makes no sense. I've attached a proposed fix that we've been running with locally.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Sep 11, 2017


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2017-09-11T23:29:47Z


Checking for a back of cell doesn't seem quite right, but perhaps. Can you please update your patch to include/modify a test case, thanks.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Sep 12, 2017


Original Redmine Comment
Author Name: Mike Popoloski
Original Date: 2017-09-12T14:01:52Z


Update with minimal test. Without my change the test fails with the previously mentioned little endian warning. With my change it does not.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Sep 12, 2017


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2017-09-12T23:03:47Z


I played a hunch and see a real bug, if you connect a e.g. 5 bit signal like this

    wire [2:0] X = 3'b110;
    foo_intf foos [3] (.x(X));
    // foos[0].x gets X[2]

Then verilator wires the bits backwards (although it seems so does one commercial simulator, while another gets it right.)

I'll fix that too.

This however brings up an important point, in the above usage it seems non-obvious that the bits connect backwards and the littleendian warning does seem appropriate. I'm pretty sure if I poll some coworkers they'll be surprised this is how it correctly connects. It's much better to write

    foo_intf foos [2:0] (.x(X));
    // foos[0].x gets X[0]

so I'm inclined to fix the connection issue but leave the message enabled. What do you think?

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Sep 13, 2017


Original Redmine Comment
Author Name: Mike Popoloski
Original Date: 2017-09-13T12:52:56Z


Hmm, interesting. I think what I would ideally want is for Verilator to warn me when connecting signals like that in a way that they would be reversed. We use arrays of interfaces quite often, but it's very rare for us to use that feature where a signal is spread out among all of the instances. That's a more involved change though, so if you want to just close this issue out that's fine with me

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Sep 13, 2017


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2017-09-13T23:11:29Z


Warning suppressed only when it shouldn't matter.

Pushed to git towards 3.911.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Sep 23, 2017


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2017-09-23T14:14:26Z


In 3.912.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
1 participant
You can’t perform that action at this time.