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
Constructors and change detection does not honor --output-split-cfuncs #1035
Comments
Original Redmine Comment Turns out the massive change detection function is partly due to some ignored (at the moment) UNOPTFLAT warnings as well as similar "circular" logic in vendor code. |
Original Redmine Comment Would you like to provide a fix, or do you think this is no longer that relevant? |
Original Redmine Comment I'll provide a fix. I hacked it up internally and it works well for us, but I'll have to put in some work to make it presentable. For AstChanged I created multiple functions with AstChanged nodes, then in the_change_request function I made it contain no AstChanged nodes and instead AstCReturn(AstLogOr(func1,func2,...) For the constructor, I changed the constructor to call a method _ctor_var_reset() and in a pass before outputting, I build that AstCFunc using a new node type AstCReset(AstVarRef) (+splitting as needed) |
Original Redmine Comment Patch that makes V3Change honor csplit-functions is available here: https://github.com/phb/verilator-dev/tree/csplit_changed |
Original Redmine Comment Splitting of variable resets in constructors: |
Original Redmine Comment I speculate you forgot to push the csplit_constructor and csplit_changed branches to github. |
Original Redmine Comment Both links work for me. The commits in question are |
Original Redmine Comment I was able to see the branches, not sure what I did wrong yesterday. JBJORK/csplit_changed:
This was a separate bug fix (thanks) so I pushed it.
Remove tests against NULL please.
Need to delete returnp. Also use "newp" instead of "n". JBJORK/csplit_constructor This is great. One of my early mistakes was to put too much stuff in Emit rather than making more Ast transformations, src/V3AstNodes.h
At present the code involved has op1 always a varref, so the accessor should be called varrefp of type NodeVarRef. src/V3VarResets.cpp +/* For each module, create a _ctor_var_reset method and add AstCReset for each variable Please use only C99 // comments.
ctor_var_reset_ to match other usages. src/V3VarResets.h +#ifndef V3CONSTRUCTORS_H I can guess what this file used to be called :) |
Original Redmine Comment Both branches updated. |
Original Redmine Comment The branches seem to change some of the same files in different ways. Please merge one into the other and let me know which one to use. Also please check t_savable passes, including turning on the split 1 flag, it's failing for me but might be the merge problem. |
Original Redmine Comment I can confirm that test is broken for me too. I must have missed it when looking over the list of failing tests (I'm developing on OSX and there are a couple of always-failing tests). I'll take care of it and upload a new patch. |
Original Redmine Comment csplit_constructor has both patches merged + fixes the t_savable issue. (Trivial fix, just a matter of a missing public: declaration) |
Original Redmine Comment Last thing I think. I noted it makes 2 functions even when disabled. Does this change make sense? diff --git a/src/V3VarResets.cpp b/src/V3VarResets.cpp
|
Original Redmine Comment It makes perfect sense. I didn't do it that way because it didn't work for V3Changed where the "main" function returns a arithmetic expression of all the "child" functions. Can you make the change? |
Original Redmine Comment Great, thanks again. Pushed to git towards 3.883. |
Original Redmine Comment In 3.884. |
Author Name: Johan Bjork
Original Redmine Issue: 1035 from https://www.veripool.org
Original Date: 2016-02-12
Original Assignee: Johan Bjork
Two files completely dominate our C++ compilation times, and the two culprits seems to be two massive functions that did not get split.
For change detection, the culprit seem to be that the AstCFunc is created during V3Changed, but the splitting happens earlier during V3Order.
For the constructor, it's emitted directly in V3EmitC and no splitting happens. (EmitCImp::emitCtorImp)
The text was updated successfully, but these errors were encountered: