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

Incorrect real parameter assignment #1427

Closed
veripoolbot opened this issue May 1, 2019 · 11 comments
Closed

Incorrect real parameter assignment #1427

veripoolbot opened this issue May 1, 2019 · 11 comments

Comments

@veripoolbot
Copy link

@veripoolbot veripoolbot commented May 1, 2019


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.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented May 1, 2019


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-05-01T17:31:52Z


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.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented May 1, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-01T22:27:37Z


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.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented May 2, 2019


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-05-02T15:25:11Z


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

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);
</code>
@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented May 2, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-02T15:49:33Z


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.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented May 2, 2019


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-05-02T20:36:12Z


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.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented May 16, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-16T22:13:09Z


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

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented May 16, 2019


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-05-16T22:43:24Z


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?

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented May 17, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-17T10:52:14Z


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

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented May 17, 2019


Original Redmine Comment
Author Name: Todd Strader (@toddstrader)
Original Date: 2019-05-17T20:54:19Z


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

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented May 18, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-05-18T00:51:18Z


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

Thanks again for the work.

@veripoolbot

This comment has been minimized.

Copy link
Author

@veripoolbot veripoolbot commented Jun 16, 2019


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2019-06-16T13:59:15Z


In 4.016.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.