Internal: Always use copied AstClass node for parameterization

[donlon]

Hi,
In V3Param, when the param processor encounters reference to parameterizable classes without parameter overrides, it copies the original AstClass node and use the uncopied one as the class instance, since the constant propagation may modify the referenced node. And as this comment said, rather than returns the copied one to the referencing node and make future copies with the original node, it returns the original one because there're some links to it that can not be traversed. But the problem probably has been fixed by #4170, #4440 and other recent patches, and I haven't observed anything wrong about it now. So perhaps we can use the other method, for the benefits that it's easier to handle node relink (it's not needed to use one more map to find the original node), and don't need to use AstClass::user4p to store the copied node. Since these two methods always need to make a copy for class references without parameter overrides, there shouldn't be any performance drawback for the new one.

[wsnyder]

The copying helps debugging - as it's much easier to understand and follow what's happening as the code currently is. I'm also worried that changes here often result in bugs. So my inclination would be unless it's breaking something to keep it as-is. Feel free to argue the other side though!

[donlon]

Thank you for commenting this! Yes I agree with you said that it may causes more bugs, since the change causes more nodes require relink. But I think such bugs can be easily discovered and fixed.

For example if all the cells/classrefs in the code have parameter override, the referencing node is initial incorrect (linked to the unparameterized one) and all of the pointers must be corrected in a certain stage. So if there were any bugs about it, it's likely that it has been revealed previously. On the other hand V3LinkDot has m_unresolvedCell/Class field used to postpone linking for some parameterized references, and V3Param has relinkDots and visit(AstVarXRef*) function to relink the parameterized nodes, so there's already mechanisms to deal with such problems and can be easily used to fix new bugs.

Another consideration is for future deparam algorithm. Currently the program knows whether a reference has overridden parameters or not because it only looks the overridden part but it's problematic (similar to #4497). For example

class myclass #(int A = $clog2(16), B = A + 4);
endclass

myclass #(.A(4)) obj1;
myclass #(.B(8)) obj2;

Here obj1 and obj2 should be of the same type (since A=4, B=8 for both of them), and the values equals to the defaults, so they're actually 'no any override'. However, to determine this requires evaluating the default value expressions with V3Const::constifyParamsEdit on the module, which adds more calculations and is not necessary. Instead we can just evaluate values which are not appeared in the parameter list, and combine them all into a parameter set, then search for previous specialized instances with the parameter set. In such scenario always copy from original module for different parameter sets (include the one equals to the default values) seems to be more straightforward.

[wsnyder]

I think I do want us to keep the original module around, as I've had too much debug pain otherwise. I'm fine with our always copying even for default parameters if that cleans things up, especially in your other pull.

[wsnyder]

@donlon let me know when you think is ready for another review.

[donlon]

I think I do want us to keep the original module around

In previous version there was still chance that the module will be removed (here), as long as not every reference has overridden parameters, so we still have to debug the code if something is not relinked to the correct module after V3Param.

Another solution is to keep the module, and just set the 'dead' flag for it, to let it survive until V3Dead. Before V3Dead V3LinkDot::linkDotParamed will relink the references to correct module.

But probably have to improve V3Dead before this (as mentioned in another PR).

[wsnyder]

Another solution is to keep the module, and just set the 'dead' flag for it, to let it survive until V3Dead

For modules, once all cells don't refer to it V3Dead should correctly clean up even without dead. You might be right dead is needed for classes currently.