Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

netlist handling of in multiple blocking assignments of same var in always_comb #993

Open
udif opened this issue May 12, 2024 · 5 comments

Comments

@udif
Copy link
Contributor

udif commented May 12, 2024

@jameshanlon
Describe the bug
It seems that when a variable is reassigned multiple times in a blocking assignment inside a combinatorial always block, all assigments are treated as the same node.

To Reproduce
Please produce a graph from this small program:

module t2 (input clk, output reg [31:0]nq);
reg [31:0]n;
always_comb begin
        n = nq;
        n = n + 1;
end

always_ff @(posedge clk)
        nq <= n;
endmodule
@jameshanlon
Copy link
Contributor

This is because there is no logic to handle multiple assignments to the same variable in a procedural block. I'll take a look at adding it.

@jameshanlon
Copy link
Contributor

I've been having a look at this issue and have come to the conclusion that it's not possible to implement the correct handling of variables in procedural blocks without performing dataflow analysis to determine the reaching definitions. This analysis is also complicated by the way parts of variables can be selected by ranges etc. So it would be a significant development of the tool to do this and beyond the scope of a bug fix or incremental feature.

@MikePopoloski I wonder if you have any thoughts on how to approach this. I know you have mentioned before plans of lowering the AST into an MLIR-based form that would be more amenable to data flow analyses. If this is the case, then it would make sense to align with that.

For the time being, I think this should be marked as a feature request rather than a bug.

@MikePopoloski MikePopoloski removed the bug label May 30, 2024
@MikePopoloski
Copy link
Owner

Ok, that's fine.

Yes, I'm hoping to be able to start looking at an MLIR layer this summer, precisely to get all of their included data flow analysis stuff for free instead of trying to reimplement it on top of the slang AST (which would be doable but clunky). There's already an effort underway to have the slang AST translated to MLIR as part of the CIRCT project, so I'll see if there's some useful overlap there.

@udif
Copy link
Contributor Author

udif commented May 31, 2024

@jameshanlon, I noticed your development branch, but haven't actually tried it yet.
I was wondering what are the specific test cases that fails and lead you to your conclusion.
I took a sample case, which your original code seem to be handling fine, even before you started your branch:

module t35;
reg [4:0]a;
always_comb
begin
        for (int i = 1; i < 5; i++)
                a[i] = a[i-1];
        a[0] = a[4];
end
endmodule

You also handle struct members (test not included here).

Handling if (and probably case) is not 100% at the moment.

module t37;
reg a, b, c;
always_comb
begin
  if (a)
          b = c;
  else
          b = a;
  c = a;
end
endmodule

I assume that all the hierarchical constructs cause no problems, and that the current issues are only with always processes.
I was looking at what subset of features needs to be supported for a useful implementation for those (by "useful" I mean handling code you expect to find in synthesizable RTL). Anything not falling under "useful" you can either warn against and quit:

  • deterministic for loops (e.g. based on constant values)
  • if and case statements
  • no break or continue (or perhaps later yes but only if it is conditioned by constants such as loop counters or parameters (e.g. i < PARAM)
  • structs
  • indexed arrays and bit ranges

I have never used LLVM or MLIR myself, so I don't know how complex would it be to use them, but I was wondering if building an in-memory flattened SystemVerilog representation of the entire process, using made-up register names for splitting blcking assignment variables, struct fields (only if not supported yet - I haven't checked) and then compile it using a separate compilation object and a separate tree walker for that AST would provide any benefit.

@jameshanlon
Copy link
Contributor

jameshanlon commented May 31, 2024

I was wondering what are the specific test cases that fails and lead you to your conclusion.

There are several aspects that I can currently see with dependencies in procedural blocks:

  1. Dependencies between multiple assignments to the same variable, where each assignment can specify a particular range.
  2. Handling depencencies in branching and reconverging control flow (ie if and case).
  3. Handling dependencies between variables used for conditional control.

It is true that the tool can properly handle ranged assignments and selections (thanks to slang's value driver logic), and this can be used to resolve multiple assignments (such as in the example on this issue). But the whole problem gets much more complicated when combined with control flow. Regarding point three, I previously made an adhoc attempt to resolve the third point for if statements in #781, but something more general is needed to handle the different constructs.

I assume that all the hierarchical constructs cause no problems, and that the current issues are only with always processes.

Yes, it is only the procedural always blocks that permit arbitrary combination of statements and assignments.

I was looking at what subset of features needs to be supported for a useful implementation for those (by "useful" I mean handling code you expect to find in synthesizable RTL).

I like this idea, and a 'netlist' tool will always be focussed on Verilog for synthesis so it makes sense that the feature set can be sensibly constrained. However, in my view if and case and their combinations in procedural blocks are important in synthesizable RTL.

I think my position is to say (for the time being at least) that the tool can only handle procedural blocks with a single assignment to a particular variable (or bit range of a variable). That sidesteps points 1 and 2. Point three can be addressed with more ad-hoc support. My old tool netlist paths was useful with this restriction, on top of there being no support for accessing sub components of variables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants