MikePopoloski / slang

SystemVerilog compiler and language services
MIT License
550 stars 117 forks source link

add support for defparam #383

Closed Winging closed 1 year ago

Winging commented 3 years ago

We have quite a few designs that used defparam. so we need to support defparam. From the spec , I noticed that the elaboration for defparam statements should follow some rules as following.

The following algorithm defines an order that produces the correct hierarchy:
a) A list of starting points is initialized with the list of top-level modules.
b) The hierarchy below each starting point is expanded as much as possible without elaborating
generate constructs. All parameters encountered during this expansion are given their final values by
applying initial values, parameter overrides, and defparam statements.
In other words, any defparam statement whose target can be resolved within the hierarchy
elaborated so far shall have its target resolved and its value applied. defparam statements whose
target cannot be resolved are deferred until the next iteration of this step. Because no defparam
inside the hierarchy below a generate construct is allowed to refer to a parameter outside the
generate construct, it is possible for parameters to get their final values before going to step c).
c) Each generate construct encountered in step b) is revisited, and the generate scheme is evaluated.
The resulting generate block instantiations make up the new list of starting points. If the new list of
starting points is not empty, go to step b).
 In order to comply with this algorithm, hierarchical names in some defparam statements will need to be
resolved prior to the full elaboration of the hierarchy. It is possible that when elaboration is complete, rules
for name resolution would dictate that a hierarchical name in a defparam statement would have resolved
differently had early resolution not been required. This could result in a situation where an identical
hierarchical name in some other statement in the same scope would resolve differently from the one in the
defparam statement.

I think this is in violation with the Diagnovisitor which travels the tree deep first following one hierachy path. Do you have any suggestions for this? By the way, we are considering submit prs for this project which we developed for quite a few months.

MikePopoloski commented 3 years ago

I haven't fully fleshed out how to support defparams but my thinking is that they will work similarly to bind directives. If the parser has noticed you're using defparams in your design, the Compilation will do a pre-pass to find them all and resolve their values before doing the normal diagnostic pass.

Winging commented 3 years ago

OK, Thanks. I will investigate and discuss in detail with you later

Winging commented 3 years ago

I have a proposal below. I'd like to see your advice.

Add a PreElaborateVisitor

The visitor takes a scope as input, it will travel all child nodes in this scope , but it only cares about parameter , module/interface instantiation . And it does not dive into generate block. After the whole travelsal , we should have all a hierarchy tree which contains defparam inside instance body.

we order the defparam statements by which the statement which modifies the root node should by in front of others , so we can instance this hierarchy tree with actual parameter.

After a first PreElaborateVisit, we begin the diagnostic visitor.

when we instantiate the tree , we should tree to see if there is a defparam override for this instance. And then we give this instance the right final parameter value.

when we see a generate block, we do a PreElaborateVisit for the scope in generate block first, so we can gather the defparam value for the paramete override of instance inside the generate block.

We continue to do Diagnostic Visitor for the scope of generate block. And elaborate the statements inside the generate scope. If there is instance instantiate , we consider the defparam override and give the right paramete value for it .

My question shows below: Could we travel the syntax tree and cares only about instance instantiation and defparam statements? I think this would be hard to do this. But I think it is also hard to do this in the existing elaboration process.

MikePopoloski commented 3 years ago

So first of all, you should generally think of the DiagnosticVisitor as being "optional", at least in the sense that it runs only after the entire tree is constructed and only to find all diagnostics. Someone can create a Compilation, access the root node, and traverse the tree themselves without running it. Therefore, the defparam handling must be done up front without involvement of the DiagnosticVisitor.

Second, I believe there needs to be a loop of attempts to resolve all defparam values over and over as long as the values are changing (or until you hit some infinite loop detector limit). If you don't have such a loop, you instead need to form a DAG based on defparam expressions referencing other parameters, which I'm not sure can be constructed. Here is an example case that illustrates some of the difficulty:

module top;
  m m1();
endmodule

module m;
  parameter a = 1;
  parameter b = 2;

  logic [b-1:0] foo;

  defparam m1.a = $bits(foo);
  defparam m1.b = 4;

  initial $display(a, b);
endmodule

All commercial simulators will print 4 4 for this.

Additionally, evaluating defparam values may require knowing types (see the $bits() call in the example) but those types can in turn change when defparams change. I imagine the defparam visitor may have to construct and throwaway Compilation objects as it tries to resolve all of the defparam values. Once you have this defparam eval loop going, you can have it descend into generate blocks as well once all defparams above them have been finalized.

Winging commented 3 years ago

From the perspective of implementation, there is also diagnostics when the ast is generating. So I think we can't fully seprate a elaboration from it. This is also a problem I encountered that there is dual diagnostics if I add a previsitor before diagnosticsVisitor.

Winging commented 3 years ago

I agree with you that we need a defparam eval loop. I am considering an elaboration of defparam. Could we just do an elaboration that only consider defparam statments/data declaration /parameter declaration/function declaration/hierachical instantiate. So we don't need to lose too much performance?

MikePopoloski commented 3 years ago

Yes, this flexibility is why the AST is designed the way it is (lazily evaluated). It should be possible to only visit the nodes needed to evaluate defparam values.

Winging commented 3 years ago

Hi Mike, I noticed that you have implemented much of the support of defparam which is much better than the solution I am implementing. I decide to follow the one you have commited. I will submit some cases that need futher consideration in this issue.

MikePopoloski commented 3 years ago

Ah, sorry about that, I missed your message earlier saying you were going to work on it. I've implemented the majority of the common case but am missing handling (and testing) of various corner cases. If you want to address those please do.

Winging commented 3 years ago

There are mainly following cases that are not supported.

defparam should not modify parameter outside that genereate block

module flop(wire logic a, wire logic b, wire logic c);
    real r1,r2;
    parameter [2:0] A = 3'h2;
    parameter B = 3'h2;
    parameter xyz = 10;
    initial begin
    r1 = A;
    r2 = B;
    $display("r1 is %f r2 is %f",r1,r2);
    end
    defparam  B = 7;
endmodule: flop

module m2;
    wire logic in1[8:0], in[8:0];
    wire logic out1[8:0];
    genvar i;
    generate
        for (i = 0; i < 6; i = i + 1) begin : somename
          flop my_flop(in[i], in1[i], out1[i]);
          defparam somename[0].my_flop.xyz = i ;
    end
    endgenerate
endmodule: m2

multiply resolution if defparam take effect which is the case in spec

module m;
    m1 n();
endmodule
module m1;
    parameter p = 2;
    defparam m.n.p = 1;
    initial $display(m.n.p);
    generate
        if (p == 1) begin : m
        m2 n();
        end
    endgenerate
endmodule
module m2;
    parameter p = 3;
endmodule

Recursive resolve of parameter . cadence and iverilog say error while vcs prints "modA aa= 13 modB aa= 12".

module modA();
    parameter AA =10;
    defparam top.b1.AA = AA + 1;
    defparam top.b1.AA = AA + 2;
    initial $display("modA aa=%d", AA);
endmodule

module modB();
    parameter AA =11;
    defparam top.a1.AA = AA + 1;
    initial $display("modB aa=%d", AA);
endmodule

module top();

    modA a1();
    modB b1();
endmodule
MikePopoloski commented 1 year ago

This should be complete now.