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
Using an array in a function called from a parameter #1315
Comments
Original Redmine Comment Unpacked arrays (the parameter) are not currently supported in constant functions. Added a test_regress/t/t_param_array3.pl test for this, will look into improving this. |
Original Redmine Comment FWIW, I ran into this issue too just now. |
Original Redmine Comment Had the same issue too. |
Original Redmine Comment I just bumped into this too. Do you think this is a tractable problem for someone new to the Verilator codebase? |
Original Redmine Comment Patrick, would love to have you help on this. While certainly it will take a bit of time investment to understand the code, it's self contained and well tested. There was a brief attempt at quickly hacking in support but it was a mess that didn't make it in, the good thing is I think we understand now a good approach. I think the only file that needs changing is V3Simulate.h. This file walks the Ast tree like a program, assigning numbers (V3Number). So if you have a constant it gets (via user2/3p) the V3Number of that constant. Then an add retrieves the number from both operands, computes, and makes a new number. To fix this bug here's what I would suggest, with a upstream merge after each step:
There's already a test, t_param_array3.pl, but might need improvements e.g. for multidimensional arrays. |
Original Redmine Comment Note #� also is the same issue, so should be part of the test cases. |
Original Redmine Comment Thanks! I'll try to poke at it and see how far I can get. |
Original Redmine Comment Wilson, Some questions regarding #1:
Patrick, are you actively looking into this? Wilson pointed out that this issue (at least part #1) relates to #�. I don't mean to dissuade you from looking at this, but I can start chewing on #1 if you haven't already. |
Original Redmine Comment The idea is every V3Number would point to the AstConst it's inside (excluding when inside the parser unless we fix that to do AstConst too, which is probably a good idea).
The rest of this bug then is would be about making some of the Simulate etc routines handle other than AstConst's (e.g. ArrayInits). Perhaps part of the first step is to rename allocNumber etc to allocValue as that seems more correct (as won't be a Number and might be e.g. eventually an array.) |
Original Redmine Comment Here is a candidate for #1: Some things to discuss: I made variants of fetch* that return AstConst* for use inside SimulateVisitor and kept V3Number* methods for use outside of SimulateVistor. This is because if something uses the AstConst outside of the simulator, we'll have lifetime problems once the simulator is destroyed and the AstConsts are cleaned up. Hopefully this works. I added an assert to make sure that the data type checks out when allocating a number and creating a new AstConst. I think this is unnecessary, but made me feel better during the refactor. Should I remove this? Please see my note in the AstConst visitor in the simulator. This refactor makes us put an AstConst under and AstConst with the same value. I think this is what we want, but it just looks a little weird. Some error messages in _bad tests changed. It seems that this change has caused some parameter values to be printed out using the data type of their port and not the data type of their numeric literal. I've included the .out file change in this branch for reference, but I'll need to figure out how to get this to go back unless you think this is actually correct or doesn't matter. We'll detect any AstConsts that found their way into the AST tree (there should be none) when the simulator deletes all the AstConsts it created. Beyond that I haven't done any leak checking. I figure it's basically the same situation as the V3Numbers that were created before, but if you think we should do more here, please let me know. Also, should I leak these AstConsts while under --debug? |
Original Redmine Comment
I would think that using an AstConst is ok also, as previously we would have
I see no harm in leaving this.
Yes, this should be correct. The alternative would be to special case
I think either version is correct in terms of the error as it doesn't say if
Think it's ok.
You mean rather than recirculate them? The old code didn't so I guess do the
These and similar were sillyness on my part, obviously the extra construction
Why virtual?
Think should be renamed allocConst.
Think should be renamed setOutValue. One might say setOutConst but I'm
assignOutValue taking const AstConst* valuep, then in future taking const AstNode* valuep.
newValue(vscp, valuep); Generally we should avoid needing to use ->num() as
These can all call textp->name() (as name() calls num().toString() already); |
Original Redmine Comment
Yeah, I tried to convert all uses of fetchNum to fetchConst but I was definitely getting some segfaults. I think the thing is that all fetchers outside of the simulator are currently making copies of the V3Numbers they get out. V3Unroll.cpp and V3Table.cpp both currently make an AstConst out of the V3Number they get out. My problem was that I initially just tried to use the AstConst* I got out of the simulator, but that didn't work since the AstConst* persisted past the simulator's lifetime and after the AstConst had been deleted. Do you think I should leave it as-is so that external users of the fetcher methods get a V3Number*? Or should I change these all to fetchConst and then make sure to clone the AstConst* outside of the simulator?
This was copy-pasted from the constant accessor. It's unnecessary so I'll remove it. But the original num() method doesn't need to be virtual either. I'll clean that up too and push it with the extra construction cleanup. Also, I don't understand the comment I copied from the other method. What does this mean?
This change:
causes USER messages to now be quoted which changes some expected messages in the test suite. I think this seems fine/maybe better. Just an FYI. I believe I've addressed everything else here: |
Original Redmine Comment
I suspect we'll end up in the long term wanting to fetch a const and clone, but we can deal with that change when we need to. The USER info stuff is an interesting consequence, I agree we'll just consider it a feature. You can push a squash of simulate_w_node_3 when you're ready. |
Original Redmine Comment Squashed, pushed and passing CI. |
Original Redmine Comment Basic support for parameter arrays is committed to git towards eventual 4.022 release. Note the example provided needs a small change to be supported, change:
to
as was more complicated to support the mix of reference types, and felt it better to get it released as-is. |
Original Redmine Comment Could you look again at #� and maybe add it to the regression? It doesn't work with the basic support that was committed to git. |
Original Redmine Comment In 4.022. |
Author Name: Marshal qiao
Original Redmine Issue: 1315 from https://www.veripool.org
Original Assignee: Wilson Snyder (@wsnyder)
Running verilator on the following code gives some errors.
Error message:
%Error: ex24.v:26: Expecting expression to be constant, but can't determine constant for FUNCREF 'calc_sums'
%Error: ex24.v:19: .. Location of non-constant ARRAYSEL: Isn't predictable Called from:
ex24.v:26: calc_sums() with parameters
The text was updated successfully, but these errors were encountered: