Major Tools
Other Tools
General Info

Issue #1035

Constructors and change detection does not honor --output-split-cfuncs

Added by Johan Bjork about 2 years ago. Updated almost 2 years ago.

% Done:



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)


#1 Updated by Johan Bjork about 2 years ago

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.

#2 Updated by Wilson Snyder about 2 years ago

  • Status changed from New to AskedReporter

Would you like to provide a fix, or do you think this is no longer that relevant?

#3 Updated by Johan Bjork about 2 years ago

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)

#4 Updated by Johan Bjork almost 2 years ago

Patch that makes V3Change honor csplit-functions is available here: I'm not sure about the V3Clean change. Let me know if that part makes no sense and if there is a better way to achieve this.

#5 Updated by Johan Bjork almost 2 years ago

Splitting of variable resets in constructors:

#6 Updated by Wilson Snyder almost 2 years ago

I speculate you forgot to push the csplit_constructor and csplit_changed branches to github.

#7 Updated by Johan Bjork almost 2 years ago

Both links work for me. The commits in question are and

#8 Updated by Wilson Snyder almost 2 years ago

I was able to see the branches, not sure what I did wrong yesterday.


+      setClean (nodep, true);

This was a separate bug fix (thanks) so I pushed it.

+      if (m_chgFuncp == NULL || v3Global.opt.outputSplitCFuncs() < m_numStmts) {

Remove tests against NULL please.

+              AstNode *n = new AstCReturn(m_scopetopp->fileline(), new AstLogOr(m_scopetopp->fileline(), callp,
+                                                                                returnp->lhsp()->unlinkFrBack()));
+              returnp->replaceWith(n);

Need to delete returnp. Also use "newp" instead of "n".


This is great. One of my early mistakes was to put too much stuff in Emit rather than making more Ast transformations, so moving towards more node-like transforms is good stuff.

+class AstCReset : public AstNodeStmt {
+    AstNode*  bodysp()        const { return op1p()->castNode(); }    // op1= varref to reset

At present the code involved has op1 always a varref, so the accessor should be called varrefp of type NodeVarRef.


+/* For each module, create a _ctor_var_reset method and add AstCReset for each variable

Please use only C99 // comments. Also please describe in the top comments a bit more how the algorighm works (see most other files).

+      if (!m_funcp) {
+          m_funcp = new AstCFunc(m_modp->fileline(), "_ctor_var_reset_" + cvtToStr(++m_funcNum), NULL, "void");

ctor_var_reset_ to match other usages.



I can guess what this file used to be called :)

#9 Updated by Johan Bjork almost 2 years ago

Both branches updated. I'm not sure what you mean with "ctor_var_reset_ to match other usages.". The leading underscore matches at least what V3Change does, if that's what you mean?

#10 Updated by Wilson Snyder almost 2 years ago

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.

#11 Updated by Johan Bjork almost 2 years ago

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.

#12 Updated by Johan Bjork almost 2 years ago

csplit_constructor has both patches merged + fixes the t_savable issue. (Trivial fix, just a matter of a missing public: declaration)

#13 Updated by Wilson Snyder almost 2 years ago

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 index 204c916..2c5cf0f 100644 --- a/src/V3VarResets.cpp +++ b/src/V3VarResets.cpp
@@ -70,9 +70,9 @@ public:
        m_modp = nodep;
        m_numStmts = 0;
        m_funcNum = 0;
-       m_funcp = NULL;
        m_tlFuncp = new AstCFunc(nodep->fileline(), "_ctor_var_reset", NULL, "void");
+       m_funcp = m_tlFuncp;

#14 Updated by Johan Bjork almost 2 years ago

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?

#15 Updated by Wilson Snyder almost 2 years ago

  • Category set to Unsupported
  • Status changed from AskedReporter to Resolved
  • Assignee set to Johan Bjork

Great, thanks again.

Pushed to git towards 3.883.

#16 Updated by Wilson Snyder almost 2 years ago

  • Status changed from Resolved to Closed

In 3.884.

Also available in: Atom