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

Using an array in a function called from a parameter #1315

Closed
veripoolbot opened this issue Jun 5, 2018 · 17 comments
Closed

Using an array in a function called from a parameter #1315

veripoolbot opened this issue Jun 5, 2018 · 17 comments
Assignees
Labels
resolution: fixed Closed; fixed type: feature-IEEE Request to add new feature, described in IEEE 1800

Comments

@veripoolbot
Copy link
Contributor


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.

 module ex24
 (
   input wire i_clk,
   input wire i_rst_n
 );
 parameter int SIZES [3:0] = '{1,2,3,4};
 typedef int calc_sums_t [3:0];
 function calc_sums_t calc_sums;
   begin
     int ii, sum = 0;
     for (ii=0; ii<4; ii++) begin
      sum = sum + SIZES[ii];
      calc_sums[ii][31:0] = sum;
     end
   end
 endfunction
 parameter int SUMS[3:0] = calc_sums();
 endmodule

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

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2018-08-25T14:37:29Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Nathan Clarke
Original Date: 2018-09-13T07:32:46Z


FWIW, I ran into this issue too just now.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: stefano cappello
Original Date: 2018-09-24T14:54:51Z


Had the same issue too.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Patrick Collins
Original Date: 2019-06-29T00:41:07Z


I just bumped into this too. Do you think this is a tractable problem for someone new to the Verilator codebase?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-06-29T09:53:38Z


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:

  1. Refactor V3Simulate.h so that rather than assigning a V3Number it assigns a AstNode* that is an AstConst*. AstConsts contain a V3Number. The functions that access the number should be able to stay in place, they would need check that the AstNode* is a AstConst. V3Simulate for speed keeps a stack of recycled V3Numbers and sometimes changes the size of them, this would change to a map of stack of AstNode* where the map index is the data type (dtypep) and it never changes the size/type of something in the stack (as they have the same data type).

  2. Improve code to make AstArrayInit (that is arrays containing constants). So this bug when it sees an AstArraySel (array selection) it would make an ArrayInit on the first assignment to an array, then set the value at the index (AstArraySel->addIndexValue) to the assignment's right hand side's AstNode (containing a const). Then when the array is referenced the AstArraySel will check the value is an AstArraySel and assuming so get the index value's nodep, then fetch the number from it.

There's already a test, t_param_array3.pl, but might need improvements e.g. for multidimensional arrays.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-06-29T10:40:30Z


Note #� also is the same issue, so should be part of the test cases.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Patrick Collins
Original Date: 2019-07-01T23:19:40Z


Thanks! I'll try to poke at it and see how far I can get.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-07-10T19:13:18Z


Wilson,

Some questions regarding #1:

  • Just to make sure I understand, are you suggesting that user2/3p would go from V3Number* to AstConst*?
  • Would allocNumber() return an AstConst* instead of a V3Number*?
  • This would propagate outside of V3Simulate.h, correct? For instance, I would expect that fetchNumber/fetchNumberNull(), which are used outside of V3Simulate.h, would return an AstConst*.
  • Is it safe to assume that any AstConst created in V3Simulate.h will eventually be linked into the AST via one of these fetchNumber users?

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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-07-10T21:58:54Z


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 V3Number destructs when AstConst desctructs.
  • user2/3p would go from V3Number* to AstConst*
  • allocNumber() would return an AstConst*
  • fetchNumber/fetchNumberNull() would return an AstConst*.
  • AstConsts in V3Simulate.h will not (generally) be put in the Ast tree, they are temporaries, just like current V3Numbers. However all will be either in the tree or deleted by the time checkTree is called, so we can have a leak/sanity check there to help debug this.

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.)

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-07-19T20:19:25Z


Here is a candidate for #1:
https://github.com/toddstrader/verilator-dev/tree/simulate-w-nodes-2

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?

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-07-22T02:34:25Z


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 would think that using an AstConst is ok also, as previously we would have
fetched an V3Number with similar lifetime.

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?

I see no harm in leaving 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.

Yes, this should be correct. The alternative would be to special case
accesses to know the (Const) node is from the original tree rather than
allocated, but that seems a misoptimization versus what we have.

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.

I think either version is correct in terms of the error as it doesn't say if
the message is before or after function parameters are resolved, however given
where the prints occur in the sources the new version seems more correct.

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.

Think it's ok.

Also, should I leak these AstConsts while under --debug?

You mean rather than recirculate them? The old code didn't so I guess do the
same thing.

 -        , m_num(V3Number(this, 32, num)) {
 +        , m_num(this, 32, num) {
 -                    V3Number fieldNum = V3Number(nump, width);
 +                    V3Number fieldNum(nump, width);

These and similar were sillyness on my part, obviously the extra construction
was pointless. Can you please put these cleanups in a separate patch, and
assuming it passes regression push them to trunk?

 +    virtual V3Number& num() { return m_num; }  // * = Value

Why virtual?

 -    V3Number* allocNumber(AstNode* nodep, uint32_t value) {
 +    AstConst* allocNumber(AstNode* nodep, uint32_t value) {

Think should be renamed allocConst.

  private:
 +    inline void setOutNumber(AstNode* nodep, const AstConst* constp) {

Think should be renamed setOutValue. One might say setOutConst but I'm
thinking ahead to the next step when we want these functions to handle more
types than just constants. If you think it's better to call these setOutConst then in the
next more general commit rename again that's fine too.

 -    void assignOutNumber(AstNodeAssign* nodep, AstNode* vscp, const V3Number* nump) {
 +    void assignOutNumber(AstNodeAssign* nodep, AstNode* vscp, const AstConst* constp) {

assignOutValue taking const AstConst* valuep, then in future taking const AstNode* valuep.
(Again looking to where we want to get to.)

 +            newNumber(vscp, constp->num());

newValue(vscp, valuep); Generally we should avoid needing to use ->num() as
much as possible, except for the numberOperates and similar visitors where we
have no choice.

 -                v3warn(USERINFO, textp->toString());
 +                v3warn(USERINFO, textp->num().toString());

These can all call textp->name() (as name() calls num().toString() already);

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-07-23T15:27:58Z


I would think that using an AstConst is ok also, as previously we would have fetched an V3Number with similar lifetime.

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?

  • virtual V3Number& num() { return m_num; } // * = Value
    Why virtual?

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?

// * = Value

This change:

-                v3warn(USERINFO, textp->toString());
+                v3warn(USERINFO, textp->num().toString());

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:
https://github.com/toddstrader/verilator-dev/tree/simulate_w_node_3

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-07-23T17:53:49Z


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?

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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-07-23T19:53:41Z


Squashed, pushed and passing CI.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-11-09T23:33:45Z


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:

  calc_sums[ii][31:0] = sum;

to

  calc_sums[ii] = sum;

as was more complicated to support the mix of reference types, and felt it better to get it released as-is.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: John Martin (@emmicro-us)
Original Date: 2019-11-10T00:27:47Z


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.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-11-10T19:28:29Z


In 4.022.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolution: fixed Closed; fixed type: feature-IEEE Request to add new feature, described in IEEE 1800
Projects
None yet
Development

No branches or pull requests

2 participants