Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2016-02-12T20:13:38Z

Ignore the comments about Typedefs, that part of the change seems to be lost somewhere along the way. I'll likely follow up with another patch that cleans up 'leftovers' in some modules. Typedefs and Modports are the two nodes I've seen lingering around in otherwise empty modules.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2016-02-22T04:17:29Z

Seems reasonable. Only obvious think I see missing is that when a scope is deleted its aboveScope's user1 should be decremented. That may then make the parent eligible for removal, but to have that happen I think the scopes need to be iterated in reverse order.

Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2016-02-22T14:02:16Z

Thanks. I'll provide an update.

We've been using an extended version of this patch internally that also removes unused typedefs and modports to get rid of even more 'empty' modules. With that patch I ran into a case where an AstNodeClassDType gets removed (And with deleteTree also it's AstMemberDType's) but some statements still referenced the individual AstMemberDType's. I didn't see any obvious checks in place to prevent this from happening (With my patch or prior) -- there is one check to avoid removing AstMemberDType's -- but as far as I can tell there is nothing that ensures that the parent stays if the children has references.
Internally I've added a specific check prior to removing the AstNodeClassDType that the refcount for each member is 0, however, I wasn't sure if this is even a state the Ast should be in. A more permanent fix might be to traverse the Ast backwards in V3Dead when it encounter a reference to AstMemberDType() and increase the refcount of the parent node?

Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2016-05-04T19:45:09Z

Complete version of the patch that expands the dead-code removal capabilities of V3Dead is available here: https://github.com/phb/verilator-dev/tree/expand_dead

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

I merged the little patch to fix V3Emit with enums.

Can you describe how you know it's ok to set packagep's to NULL?

A few nits.

Comparing against NULL isn't typical.

•   if (!nodep->isTop() && nodep->varsp() == NULL && nodep->blocksp() == NULL && nodep->finalClksp() == NULL) {
  

•   if (!nodep->isTop() && !nodep->varsp() && !nodep->blocksp() && !nodep->finalClksp()) {
  

At present all dump names are lower case, so update the dumpCheck's to lower case e.g. deadModules.

Please rename m_scsp to m_scopesp to match other files.

I presume you looked through all AstNode types and made sure you're hitting all packagep()'s etc.

Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2016-05-06T14:17:00Z

Thanks for the comments

I've updated the commit with details on why removing packagep() should be safe and details on what nodes have the package links. Basically packagep is mostly used in V3LinkDot and is not used at all after V3Scope (but the links would prevent cleanup of dead nodes).
The updated version also fixes the nits mentioned.

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2016-05-07T02:27:11Z

I'm not sure if I like having the deletion of the packagep's here versus LinkDot cleaning up after itself, but I don't really like adding more to LinkDot either, so I guess leave it.

Please add VL_DANGLING(nodep)s some are missing. They go on the same line after each pushDeletep or deleteTree. (I sometimes grep for deletes, so it helps to have them on the same line)

All the ugly nodep->user1(nodep->user() - 1) looked ugly and are error prone. Please merge from master and now use e.g. nodep->user1Inc(-1).

I saw a bunch more changes, do you think it's ready to merge with the above?

Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2016-05-09T12:59:00Z

All the open patches should merge fine, they don't overlap. I'm not happy with the packagep() stuff either. Let me see if I can find some other place to put it that makes more sense.

Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2016-05-11T13:11:43Z

updated version here: https://github.com/phb/verilator-dev/tree/expand_dead -- packagep() removal is still left in V3Dead, couldn't find any better location for it.

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

Great! Merged it all with a few trivial whitespace things.

Pushed to git towards 3.883.

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

In 3.884.