Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2016-02-16T15:56:43Z

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
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2016-02-22T03:34:11Z

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

Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2016-02-22T13:42:44Z

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
Author Name: Johan Bjork
Original Date: 2016-05-04T14:33:00Z

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.

Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2016-05-04T15:21:16Z

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

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2016-05-06T03:15:34Z

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

Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2016-05-06T13:28:41Z

Both links work for me. The commits in question are
phb/verilator-dev@d46ada3 and phb/verilator-dev@a8e38a7

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2016-05-07T01:49:57Z

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

Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2016-05-11T16:58:51Z

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?

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2016-05-12T01:30:58Z

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
Author Name: Johan Bjork
Original Date: 2016-05-12T02:45:50Z

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
Author Name: Johan Bjork
Original Date: 2016-05-12T02:54:40Z

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

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2016-05-12T03:21:10Z

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

Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2016-05-12T03:37:56Z

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
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2016-05-12T11:19:52Z

Great, thanks again.

Pushed to git towards 3.883.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2016-05-19T01:18:09Z

In 3.884.