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
Incorrect real parameter assignment #1427
Comments
Original Redmine Comment 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. |
Original Redmine Comment 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:
|
Original Redmine Comment Multiple follow-up questions:
Sorry, I'm not following this. If I do this:
I get:
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?
Aren't some form of buckets going to be necessary if the key to m_valueMap is a hash of something?
By this do you mean we should continue doing this case?
|
Original Redmine Comment
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.
Maybe, just not sure why specific number of BUCKETS is required.
Yes, and the returned value for a given design should be the same. |
Original Redmine Comment Almost done, but still a WIP: 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. |
Original Redmine Comment 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). |
Original Redmine Comment 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? |
Original Redmine Comment 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). |
Original Redmine Comment Thanks again for the feedback. I've updated the branch to reflect all of this: |
Original Redmine Comment Pushed to git towards 4.015. Just replaced two minor C++11 isms. Thanks again for the work. |
Original Redmine Comment In 4.016. |
Author Name: Todd Strader (@toddstrader)
Original Redmine Issue: 1427 from https://www.veripool.org
Original Assignee: Todd Strader (@toddstrader)
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.
The text was updated successfully, but these errors were encountered: