povik / yosys-slang

SystemVerilog frontend for Yosys
ISC License
55 stars 7 forks source link

Elaboration errors not being catched unless `--dump-ast` is specified #32

Closed alikates closed 3 months ago

alikates commented 3 months ago

I'm replicating the issues being faced when parsing BSC core_tile by writing small tests for them. I noticed some errors are only reported when I specify --dump-ast. Otherwise the frontend goes past the elaboration and hard-fails in the middle of generating the netlist.

For example, this code:

package test_pkg;
typedef enum logic [1:0] {
    A,    B,    C,    D
} port_enum_t;
endpackage

module port_enum(
    input test_pkg::port_enum_t enum_in,
    output logic [1:0] bits_out );
    assign bits_out = enum_in;
    always_comb begin
        assert(enum_in === bits_out);
    end
endmodule

module port_enum_assign(
    input logic                 sel,
    input test_pkg::port_enum_t enum_in,
    input logic [1:0]           bits_in,
    output logic [1:0]          bits_out
);
    port_enum port_enum_inst (
        .enum_in(sel ? port_enum_t'(bits_in) : enum_in),
        .bits_out(bits_out) );
endmodule

module t01 (sel, enum_in, bits_in, bits_out);
    input logic sel;
    input logic [1:0] enum_in;
    input logic [1:0] bits_in;
    output logic [1:0] bits_out;

    port_enum_assign t (
        .enum_in(test_pkg::port_enum_t'(enum_in)),
        .* );
endmodule

Without --dump-ast:

yosys> read_slang tests/selects/port_enum.sv
1. Executing SLANG frontend.
<suppressed ~12 debug messages>
ERROR: Assert `internal_signal.size() == signal.size()' failed in slang_frontend.cc:2256.

With it:

yosys> read_slang tests/selects/port_enum.sv --dump-ast

(AST)

Top level design units:
    t01

tests/selects/port_enum.sv:30:24: error: reference to non-constant variable 'port_enum_t' is not allowed in a constant expression
        .enum_in(sel ? port_enum_t'(bits_in) : enum_in),
                       ^~~~~~~~~~~
tests/selects/port_enum.sv:30:24: note: declared here
        .enum_in(sel ? port_enum_t'(bits_in) : enum_in),
                       ^
Build failed: 1 error, 0 warnings
ERROR: Compilation failed

I'm not sure if this really belongs here or it's an error with slang. From what I understand there's isn't any branch in the codepath between the AST dump and the check for the failed elaboration. So maybe we are passing a different configuration to slang in each case, and depending on that it reports the errors or not? Btw, this is with the staging branch.

povik commented 3 months ago

That's a good catch and thanks for minimizing this.

From what I understand there's isn't any branch in the codepath between the AST dump and the check for the failed elaboration. So maybe we are passing a different configuration to slang in each case, and depending on that it reports the errors or not?

Some facts on the AST are derived on demand (e.g. in the getType() method of symbols), and my bet is that the forceElaborate call is not strong enough to have everything elaborated ahead of the hasIssuedErrors check, so dumping the AST helped. As I see it this may be a bug in the forceElaborate() method.

povik commented 3 months ago

As I see it this may be a bug in the forceElaborate() method.

It looks like forceElaborate() purposefully doesn't enter instance bodies so we are using it wrong (it can only be used on one instance at a time).

povik commented 3 months ago

I changed the way we are using forceElaborate in f65e737.