Project

General

Profile

[logo] 
 
Home
News
Activity
About/Contact
Major Tools
  Dinotrace
  Verilator
  Verilog-mode
  Verilog-Perl
Other Tools
  BugVise
  CovVise
  Force-Gate-Sim
  Gspice
  IPC::Locker
  Rsvn
  SVN::S4
  Voneline
  WFH
General Info
  Papers

Issue #1035

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

Added by Johan Bjork almost 2 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Unsupported
% Done:

0%


Description

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)

History

#1 Updated by Johan Bjork almost 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 almost 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 almost 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 over 1 year ago

Patch that makes V3Change honor csplit-functions is available here: https://github.com/phb/verilator-dev/tree/csplit_changed 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 over 1 year ago

Splitting of variable resets in constructors: https://github.com/phb/verilator-dev/tree/csplit_constructor

#6 Updated by Wilson Snyder over 1 year ago

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

#7 Updated by Johan Bjork over 1 year ago

Both links work for me. The commits in question are https://github.com/phb/verilator-dev/commit/d46ada3c334bc8bd32af4e778387197cac338162 and https://github.com/phb/verilator-dev/commit/a8e38a77c6875f4b598cf40cb5f733b742bc4c54

#8 Updated by Wilson Snyder over 1 year ago

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

JBJORK/csplit_changed:

+      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".

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, so moving towards more node-like transforms is good stuff.

src/V3AstNodes.h
+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.

src/V3VarResets.cpp

+/* 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.

src/V3VarResets.h

+#ifndef V3CONSTRUCTORS_H

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

#9 Updated by Johan Bjork over 1 year 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 over 1 year 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 over 1 year 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 over 1 year 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 over 1 year 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_tlFuncp->declPrivate(true);
        m_tlFuncp->isStatic(false);
        m_tlFuncp->slow(true);
+       m_funcp = m_tlFuncp;
        m_modp->addStmtp(m_tlFuncp);

#14 Updated by Johan Bjork over 1 year 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 over 1 year 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 over 1 year ago

  • Status changed from Resolved to Closed

In 3.884.

Also available in: Atom