Project

General

Profile

[logo] 
 
Home
News
Activity
About/Contact
Major Tools
  Dinotrace
  Verilator
  Verilog-mode
  Verilog-Perl
Other Tools
  IPC::Locker
  Parallel::Forker
  Voneline
General Info
  Papers

Issue #1465

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

Added by Alex Chadwick 5 months ago. Updated 5 months ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
TranslationError
% Done:

0%


Description

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

verilator-savable.patch View (4.12 KB) Alex Chadwick, 06/14/2019 08:58 AM

verilator-savable(1).patch View (4.86 KB) Alex Chadwick, 06/14/2019 01:39 PM

History

#1 Updated by Wilson Snyder 5 months ago

  • Status changed from New to AskedReporter
  • Assignee set to Alex Chadwick

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!

#2 Updated by Alex Chadwick 5 months ago

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?

#3 Updated by Wilson Snyder 5 months ago

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.

#4 Updated by Alex Chadwick 5 months ago

Changes made, please find the new patch attached.

#5 Updated by Wilson Snyder 5 months ago

  • Status changed from AskedReporter to Resolved

Thanks much.

Pushed to git towards 4.015.

#6 Updated by Wilson Snyder 5 months ago

  • Status changed from Resolved to Closed

In 4.016.

Also available in: Atom