[slang] Fix static subroutine local variables multiple assigns and reduce MultipleAlwaysAssigns diag severity

[likeamahoney]

MultipleAlwaysAssign error diag check is not handle such type of static function local variable assign despite the fact that there is an function local variable c assigns during call at always_ff block:

module top(input clk, input reset);
    function logic [3:0] m(logic d); // static
        logic c;   // static
        c = d;
        return c;
    endfunction

    logic a, b;

    always_ff @(posedge clk) begin
        a <= m(a);
    end

    always @(posedge reset) begin
        b <= m(a);
    end
endmodule

But such type of code triggers that diag correctly:

module top(input clk, input reset);
    function logic [3:0] m(logic d); // static
        logic c;   // static
        c = d;
        return c;
    endfunction

    logic a, b;

    always_ff @(posedge clk) begin
        a <= m(a);
    end

    always @(posedge reset) begin
        a <= m(a);
    end
endmodule

IEEE Std 1800™-2017 says:

Variables on the left-hand side of assignments within an always_ff procedure,
including variables from the contents of a called function, shall not be written to by any other process.

So that thing tell us to maintain function local variables just like as procedure block outs especially in the case of a static function. I don't think so that is need to be fixed in case of automatic function.

Secondary i want to suggest to reduce severity of MultipleAlwaysAssing to have an opportunity for disabling it manually because it's hard to make it heuristically correct at all cases which leads to false positives. For example:

module m(input clk);

logic i;

always_ff @(posedge clk) begin
    i <= 1;
end

always_ff @(negedge clk) begin
    i <= 2;
end

endmodule

Without synthesis it's hard to understand which paths are opposite.

Here are an example of correctly synthesiable code of RISCV kernel from https://github.com/syntacore/scr1

Which is triggers that diag - first_always_ff_call and second_always_ff_call.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.64%. Comparing base (4c4155f) to head (09eb208).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #903   +/-   ##
=======================================
  Coverage   93.64%   93.64%           
=======================================
  Files         189      189           
  Lines       47298    47297    -1     
=======================================
+ Hits        44290    44292    +2     
+ Misses       3008     3005    -3     

Files	Coverage Δ
source/ast/expressions/CallExpression.cpp	93.38% <100.00%> (-0.02%)	⬇️

... and 2 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c4155f...09eb208. Read the comment docs.

[MikePopoloski]

Can you explain more why you want to downgrade the error? The LRM is pretty clear that such code is not allowed, and the example links you provided don't tell me much; each link seems to be for two different variables entirely that are only ever driven by one always_ff block that I can see.

[likeamahoney]

and the example links you provided don't tell me much; each link seems to be for two different variables entirely that are only ever driven by one always_ff block that I can see.

Thanks for the response!

In the syntacore example above slang errors on the static local function (which called in both always_ff blocks) variable tmp assignment.

[MikePopoloski]

Hmm, I see. All of the tools I tried have interpreted this rule to not include local variable declarations, so I think I would just rather change slang to not cause an error if the variable being written to is a local variable of a task or function.

[likeamahoney]

Hmm, I see. All of the tools I tried have interpreted this rule to not include local variable declarations, so I think I would just rather change slang to not cause an error if the variable being written to is a local variable of a task or function.

Maybe we shouldn’t change slang in this regard, but just hide that error under the --compat vcs option, since LRM still mentions local function variables inside calls at always_ff procedural blocks.

[likeamahoney]

I submit an issue for syntacore RISC V kernel project - syntacore/scr1#63

[MikePopoloski]

I ended up fixing this differently:

• The issue where the error was not reported when one of the blocks was a plain always block is fixed in 97a57fb
• I added a new compat flag that allows multiple drivers to subroutine local variables in bfad52e