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 #1427

Incorrect real parameter assignment

Added by Todd Strader 21 days ago. Updated 4 days ago.

Status:
Resolved
Priority:
Normal
Assignee:
Category:
Configure/Make/Compiling
% Done:

0%


Description

See here for the test and a possible fix: https://github.com/toddstrader/verilator-dev/tree/broken_real_param

The problem was that ParamVisitor::paramValueNumber() relies on nodes being passed into it having stable pointers (as described by the comment). However, in this case you can see that while this method is called repeatedly with different real values (as expected) the AstNode that comes in is always the same.

Switching the map to <string, int> fixes the problem and passes all regression tests. However, I don't know what it does to the runtime and I don't know if nodep->name() is guaranteed to be unique in all cases here.

Please let me know how you'd like to proceed.

History

#1 Updated by Todd Strader 21 days ago

So I found one of the cases above that I was speculating about. I haven't fully determined why, but there are cases where the above patch causes internal errors.

I have found that I can work around all of this by remembering both the AstNode* and nodep->name() and creating a new hash value if either of these are different. But at this point, I'm wondering if it's better to get whatever is recycling AstNodes when evaluating constant real value to stop doing that.

Please let me know what you think.

#2 Updated by Wilson Snyder 20 days ago

  • Status changed from New to Assigned

The duplicate nodes are because the values are being evaluated in the loop and previously deleted nodes reused. I suppose in theory we could defer deletion, but that seems to paper over the bigger design flaw which is use of nodep in the first place.

Some issues/notes:

  • The paramValueNumber routine originally only supported numbers (as parameters were numbers). Using name() might have been sane at one point but it is now also used with interfaces and parameter type. I don't see how I thought this would work correctly with multiple arrays (with different values).
  • I think what you can do is always use V3Hash of the node (not node's name) so that we'll get an expression that is unique.
  • I can't explain why I thought the BUCKETS mess was necessary.
  • paramValueNumber sometimes adds a "z" (when first called) and then later doesn't (when hits). This should be fixed to always add "z" so fewer modules get created.
  • Ideally try to preserve the previous version's naming for a small number of integer parameters.
  • Nit: Please rename t_real_param to t_param_real.

#3 Updated by Todd Strader 20 days ago

Multiple follow-up questions:

I think what you can do is always use V3Hash of the node (not node's name) so that we'll get an expression that is unique.

Sorry, I'm not following this. If I do this:
string paramValueNumber(AstNode* nodep) {
    V3Hash hash(nodep);
    UINFO(1, hash.m_both<<" -- "<<nodep<<endl);
I get:
- V3Param.cpp:152:    26756959 -- CONST 0x557f11981220 <e908#> {f24} @dt=0x557f11978db0@(d)  10.5
- V3Param.cpp:152:    26756959 -- CONST 0x557f11981220 <e925#> {f24} @dt=0x557f1197b6d0@(d)  11.5
- V3Param.cpp:152:    26756959 -- CONST 0x557f11981220 <e942#> {f24} @dt=0x557f11978820@(d)  20.5
- V3Param.cpp:152:    26756959 -- CONST 0x557f11981220 <e954#> {f24} @dt=0x557f11980b00@(d)  21.5

Which makes sense because as far as I can tell, V3Hash is just doing some pointer munging on nodep which doesn't change even though the contents do. There's a comment in V3Hash about "A hash of a tree of nodes", but I don't see V3Hash doing any tree things. What am I missing here?

I can't explain why I thought the BUCKETS mess was necessary.

Aren't some form of buckets going to be necessary if the key to m_valueMap is a hash of something?

Ideally try to preserve the previous version's naming for a small number of integer parameters.

By this do you mean we should continue doing this case?
longname += "_" + paramSmallName(srcModp, modvarp) + exprp->num().ascii(false);

#4 Updated by Wilson Snyder 20 days ago

Which makes sense because as far as I can tell, V3Hash is just doing some pointer munging on nodep

I had forgotten how this works, I shouldn't have said use V3Hash directly, instead look for example uses of V3Hashed, e.g. in V3SenTree. Basically create a hashed object then call hashed->uncachedHash(nodep) to get from hashed an identifier summarizing the contents of the tree. It is supposed to never care about the node pointer values as the intent is to look for duplicate contents.

Aren't some form of buckets going to be necessary if the key to m_valueMap is a hash of something?

Maybe, just not sure why specific number of BUCKETS is required.

Ideally try to preserve the previous version's naming for a small number of

integer parameters.

By this do you mean we should continue doing this case?

Yes, and the returned value for a given design should be the same.

#5 Updated by Todd Strader 20 days ago

Almost done, but still a WIP: https://github.com/toddstrader/verilator-dev/tree/broken_real_param2

Using a <V3Hash,int> map resolves this issue. But it clearly breaks down when hash collisions occur. To test this, I forced collisions on the V3Hash. I left this in as a commented-out line to document what I did.

Since storing the entire AST tree to check for collisions sounds pretty expensive, I am just storing the name. Right now, if there is a collision I will forget the older value which means if the forgotten value comes back around it will get a new number. I'm assuming this is will not break anything and just introduce a slight performance hit in the rare case of a collision. Is that correct? Also, do you think it would be better to keep the older value and drop the new one in the case of a collision?

When I start using V3Hashed in paramValueNumber() I get compile errors from a bunch of interface tests including t_interface_arraymux because AstIfaceRefDType doesn't have a sameHash(). I don't understand what this is doing or why some nodes' sameHashes are just V3Hash() and others are hashing their members. I added a sameHash() to AstIfaceRefDType and this has gotten rid of all the compile errors, but I'm now getting a few runtime errors where it looks like one interface is being confused for a different one (e.g. t_interface_gen2). So my blind hack clearly didn't work. Can you point me in the right direction here?

Lastly, do you want me to keep the commented out line for collision testing? I don't believe there is any way to automate this short of building a special binary or introducing a command line flag that no one should ever use. When we get everything else straight I can run the regression suite with and without the line just to check everything out once.

#6 Updated by Wilson Snyder 5 days ago

Sorry, forgot this one needed reply.

On a collision not reusing sounds fine, I'd keep new one.

For collision testing seems safest to add a new undocumented option e.g. --debug-collision and use that. Then make a t_param_real2_collision.pl that uses it and checks. Also (once) run all tests with that, if a few still fail also make _collision.pl tests for those that failed (assuming small number, whatever seems appropriate for good test coverage).

#7 Updated by Todd Strader 5 days ago

Great, thanks for the feedback. I haven't had any more time to look at this once since I last posted, so I'm still clueless on sameHash(). Do you have any advice on that method wrt AstIfaceRefDType?

#8 Updated by Wilson Snyder 5 days ago

I looked through and am a bit fuzzy on why interfaces are handled this way. I think the idea is that each parameterized interface when used as a call must result in a different cell. Assuming that's right I think you want to use the hash of the interface's module (e.g. the deparameterized hash).

V3Hash exists for objects where identicality is to be determined, otherwise it wasn't present. In general all members of the Ast for that object which are needed to determine identicality should be included (e.g. name alone is enough for a Var, for a constant need the value and width of the value, etc).

#9 Updated by Todd Strader 5 days ago

Thanks again for the feedback. I've updated the branch to reflect all of this: https://github.com/toddstrader/verilator-dev/tree/broken_real_param2

#10 Updated by Wilson Snyder 4 days ago

  • Status changed from Assigned to Resolved

Pushed to git towards 4.015. Just replaced two minor C++11 isms.

Thanks again for the work.

Also available in: Atom