Project

General

Profile

[logo] 
 
Home
News
Activity
About/Contact
Major Tools
  Dinotrace
  Verilator
  Verilog-mode
  Verilog-Perl
Other Tools
  BugVise
  CovVise
  Force-Gate-Sim
  Gspice
  IPC::Locker
  Rsvn
  SVN::S4
  Voneline
  WFH
General Info
  Papers

Issue #1315

Using an array in a function called from a parameter

Added by Marshal qiao about 1 year ago. Updated about 1 month ago.

Status:
Feature
Priority:
Normal
Assignee:
-
Category:
Unsupported
% Done:

0%


Description

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

History

#1 Updated by Wilson Snyder about 1 year ago

  • Category set to Unsupported
  • Status changed from New to Confirmed

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.

#2 Updated by Wilson Snyder about 1 year ago

  • Status changed from Confirmed to Feature

#3 Updated by Nathan Clarke 12 months ago

FWIW, I ran into this issue too just now.

#4 Updated by stefano cappello 11 months ago

Had the same issue too.

#5 Updated by Patrick Collins about 2 months ago

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

#6 Updated by Wilson Snyder about 2 months ago

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.

#7 Updated by Wilson Snyder about 2 months ago

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

#8 Updated by Patrick Collins about 2 months ago

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

#9 Updated by Todd Strader about 2 months ago

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 bug1305. I don't mean to dissuade you from looking at this, but I can start chewing on #1 if you haven't already.

#10 Updated by Wilson Snyder about 2 months ago

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

#11 Updated by Todd Strader about 1 month ago

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?

#12 Updated by Wilson Snyder about 1 month ago

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

#13 Updated by Todd Strader about 1 month ago

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 fetch*Num* to fetch*Const* 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 fetch*Const* 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

#14 Updated by Wilson Snyder about 1 month ago

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 fetch*Const* 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.

#15 Updated by Todd Strader about 1 month ago

Squashed, pushed and passing CI.

Also available in: Atom