--savable generates invalid c++ for some packed arrays
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
#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.)
#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
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
Also available in: Atom