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

--savable generates invalid c++ for some packed arrays #1465

Closed
veripoolbot opened this issue Jun 14, 2019 · 6 comments
Closed

--savable generates invalid c++ for some packed arrays #1465

veripoolbot opened this issue Jun 14, 2019 · 6 comments

Comments

@veripoolbot
Copy link

@veripoolbot veripoolbot commented Jun 14, 2019


Author Name: Alex Chadwick (@Chadderz121)
Original Redmine Issue: 1465 from https://www.veripool.org

Original Assignee: Alex Chadwick (@Chadderz121)


When trying to use @--savable@ I noticed compiler errors in the generated C++ code for the save/restore routines. This seems to be the result of improper code being generated for some types of variable; notably multidimensional packed arrays. I'm not familiar with verilator's internals, but I've attached a patch which I believe should work in all cases. I have tried to mirror the code that declares types to generate appropriate save/restore routines accordingly. The only thing I'm not clear on is whether the code I've written will work properly for opaque types like strings, but it does seem to in practice. I've also extended the regression test @t_savable@ to include multidimensional packed arrays.

Thanks in advance,
Alex

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jun 14, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-06-14T11:11:14Z


First thanks for making a patch and improving the tests. The patch seems reasonably close, just a few little things.

  • Not sure why you're testing for width being <= 16, there should be nothing special about size 16. I think you should be using approx "basicp && basicp->keyword() != STRING && basicp->isWide())

  • It seems to work, but other places don't do assignments in VN_CAST and seems somewhat risky on how it is implemented, so please pull the "basicp= part" out of VN_CAST to a line above the cast.

  • In the patch please insert your name in docs/CONTRIBUTORS to acknowledge this and future contributions are made under the Developer Certificate of Origin (https://developercertificate.org/). (Needed just once.)

Thanks!

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jun 14, 2019


Original Redmine Comment
Author Name: Alex Chadwick (@Chadderz121)
Original Date: 2019-06-14T11:34:21Z


I see yes, I never bothered to look at the definition of @isWide@ etc, I was just mirroring the code in @emitVarDecl@ to ensure the decision would match. Certainly what I wrote looks equal to @baiscp->isWide()@.

I'm note sure the @string@ part is going to work; I believe we don't know at this point if @basicp@ is a @BasicDType@ or a @PackArrayDType@ (this was one of the mistakes the old code failed to consider). I suppose we could attempt to cast to a @BasicDType@ and check the @Keyword@ if that succeeds. I'm not sure if a packed array of strings is possible, but that would not work if so. As I mentioned the patch I submitted works in the test case (which includes a @string@ variable) so I'm not sure if it's necessary to check explicitly.

As for the assignments in @VN_CAST@, are you happy with use of the comma operator to take care of this or would you prefer the more verbose but potentially clearer breaking into multiple statements outside the @for@?

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jun 14, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-06-14T12:44:12Z


Strings can't be packed arrays, so we're probably ok there. If you're ambitious try adding an array of strings to the test, but might cause other breakage.

Yes, please go for multiple statements in the VN_CAST.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jun 14, 2019


Original Redmine Comment
Author Name: Alex Chadwick (@Chadderz121)
Original Date: 2019-06-14T13:39:27Z


Changes made, please find the new patch attached.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jun 14, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-06-14T22:42:49Z


Thanks much.

Pushed to git towards 4.015.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jun 16, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-06-16T13:59:46Z


In 4.016.

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.