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

avoid dynamic_cast for trivial casts #1021

Closed
veripoolbot opened this issue Dec 25, 2015 · 4 comments
Closed

avoid dynamic_cast for trivial casts #1021

veripoolbot opened this issue Dec 25, 2015 · 4 comments
Labels
resolution: fixed Closed; fixed

Comments

@veripoolbot
Copy link
Contributor


Author Name: Johan Bjork
Original Redmine Issue: 1021 from https://www.veripool.org
Original Date: 2015-12-25
Original Assignee: Johan Bjork


In most compilers dynamic_cast boils down to (at least) one string comparison. This showed up as a large bottleneck when verilating a medium/large sized design.
For the cases where the type being casted to has no subclasses, we can simply re-use AstNode::type() and perform a static_cast.

This reduces verilation time on one of our designs by 10 seconds, (~60s->~50s)
https://github.com/phb/verilator-dev/tree/less_dyncast

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2016-01-07T01:50:34Z


No compiler I am aware of uses a string compare, the time delta comes from determining the child arangements. Anyhow the improvement is still good. I was a bit afraid that there might be some e.g. AstNUser cases where I had assumed that the data could take on more than just a node type (where this thus wouldn't work), but a audit didn't turn anything up, so I think this is clean.

Fixed in git towards 3.881.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2016-02-04T03:31:37Z


Note I may have to back this out due to #� - it's illegal to test "this" for NULL, and it's important for code readability that pointer->castFoo work with pointer being null. So I think dynamic_cast must be used to be legal C.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2016-02-04T04:43:13Z


I didn't quite realize what amount of work that new warning would entail, thanks for looking into it. Didn't know it was undefined until clang printed the warning. Anyway, I looked at why exactly doing the this compare is incorrect, and according to the standard it seems that undefined behaviour is caused by invoking a method on a null object, not the actual null compare, so I guess both this and the previous version would be undefined according to the standard for a null object.
I'm obviously fine if you want to revert it for now, but might be worth thinking about another implementation that does not rely on calling methods on null objects.
One idea would be to generate overloaded functions for each node type, i.e.,
Cast(nodep) for each type of node type. The implementation should be pretty much identical to the current one.

@veripoolbot
Copy link
Contributor Author


Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2016-03-02T00:15:36Z


In 3.882.

@veripoolbot veripoolbot added the resolution: fixed Closed; fixed label Dec 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolution: fixed Closed; fixed
Projects
None yet
Development

No branches or pull requests

1 participant