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 #1034

Extend V3Dead to be able to remove empty package modules/cells

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

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

0%


Description

Deletes Scopes, Typedefs, Cells pointing to empty modules and finally cells. https://github.com/phb/verilator-dev/tree/deadstuff

History

#1 Updated by Johan Bjork almost 2 years ago

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.

#2 Updated by Wilson Snyder over 1 year ago

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.

#3 Updated by Johan Bjork over 1 year ago

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?

#4 Updated by Johan Bjork over 1 year ago

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

#5 Updated by Wilson Snyder over 1 year ago

  • Category set to Unsupported
  • Assignee set to Johan Bjork

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.

#6 Updated by Johan Bjork over 1 year ago

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.

#7 Updated by Wilson Snyder over 1 year ago

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?

#8 Updated by Johan Bjork over 1 year ago

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.

#9 Updated by Johan Bjork over 1 year ago

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.

#10 Updated by Wilson Snyder over 1 year ago

  • Status changed from New to Resolved

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

Pushed to git towards 3.883.

#11 Updated by Wilson Snyder over 1 year ago

  • Status changed from Resolved to Closed

In 3.884.

Also available in: Atom