Add new diagnostic: Register only assigned on reset

[Sustrak]

Added a new warning which triggers when a register is only assigned while the design is on reset.

[jrudess]

I liked the idea of this diagnostic, so I gave it a spin on some projects. It has a small issue with structs.

struct.sv:10:9: warning: register 's' is only assigned on reset. This will cause warnings or errors in synthesis tools [-Wonly-assigned-on-reset]
    s_t s;
        ^

module top;
    logic clk;
    logic rst_b;
    logic ns, ps;

    typedef struct packed {
        logic a;
        logic b;
    } s_t;
    s_t s;

    always_ff @(posedge clk, negedge rst_b) begin
        if (~rst_b)         s <= '0;
        else if (ns == 0) s.a <= 1'b1;
        else if (ps == 0) s.b <= 1'b1;
    end

endmodule

[jrudess]

Similar variant for packed arrays:

packed.sv:8:22: warning: register 'fifo' is only assigned on reset. This will cause warnings or errors in synthesis tools [-Wonly-assigned-on-reset]
    logic [4:0][2:0] fifo;
                     ^

module top;
    logic clk;
    logic rst_b;
    logic push;
    logic ptr;
    logic [2:0] data;

    logic [4:0][2:0] fifo;

    always_ff @(posedge clk or negedge rst_b) begin
        if (~rst_b) fifo <= '0;
        else if (push) begin
            for (int i = 0; i <= 4; i++) begin
                if (ptr == i) fifo[i] <= data;
            end
        end
    end
endmodule

[MikePopoloski]

Thanks for the PR. These kinds of checks are something that I hope slang can make very easy to add, so I'm happy to see your attempt here. I'll leave a few comments on the implementation itself, but also wanted to discuss at a higher level where things like this should live.

The check as-is is far too specific to be something included in a normal compilation run that's on for all codebases. Someone might so happen to have code that triggers this warning but is otherwise perfectly valid, which can be pretty annoying, and it will be difficult to scale this up for every kind of check people will want to add. Instead I think we want checks like this to live in a separate tool, like clang's "clang-tidy" tool. This shouldn't be very hard to do; your check right now doesn't actually depend on living inside the Compilation class and could just be placed outside of it. Such a tool could then have as many checks as people care to add, and we can group them into checks that are applicable to synthesis, simulation, performance, etc.

Also the code you have here isn't too hard to follow IMO, but I bet it would still benefit from having an API in slang like clang's "AST Matchers" to make it even easier and more succinct to add checks like this.

[Sustrak]

Hi @jrudess @MikePopoloski thanks for the feedback. I'll check and correct the mistakes I've done in the implementation of my code, that you both have pointed out. Very appreciated for that.

Regarding

I think we want checks like this to live in a separate tool, like clang's "clang-tidy" tool.

Do you think this tool could live in this repo in a similar way the driver does? Maybe with the name slang-tidy?

Also

I bet it would still benefit from having an API in slang like clang's "AST Matchers" to make it even easier and more succinct to add checks like this.

I also think it would be very nice to have. I can try to generalize some parts of my code in order to start implementing some of these "AST Matchers"

One again, thanks for the feedback

salut!

[MikePopoloski]

Do you think this tool could live in this repo in a similar way the driver does? Maybe with the name slang-tidy?

Yes, that sound good to me. If you want to try that feel free.

I also think it would be very nice to have. I can try to generalize some parts of my code in order to start implementing some of these "AST Matchers"

That would be good. One nice thing about the way clang does it is that you can register a bunch of matchers and then run the find operation once, so that adding more than one of these checks doesn't turn this into a big O(n^2) problem.

[Sustrak]

Hi, I'm closing this MR since I'll try to better implement the solution with the ideas mentioned above.