MikePopoloski / slang

SystemVerilog compiler and language services
MIT License
554 stars 120 forks source link

foreach implicit iterator declaration fails for multi-dimensional arrays if previous dimensions have an index that is explicitly declared #436

Closed jrudess closed 2 years ago

jrudess commented 2 years ago

This example works in VCS.

module top;
    int array[8][8];
    initial begin
        foreach (array[i]) begin
            foreach (array[i][j]) begin
                array[i][j] = i * j;
            end
        end
    end
endmodule

But it produces the following errors with slang

slangtest.sv:5:31: error: use of undeclared identifier 'j'
            foreach (array[i][j]) begin
                              ^
slangtest.sv:5:33: error: expected '['
            foreach (array[i][j]) begin
                                ^

Arguably, the better syntax to use would be as follows, but I'm attempting to parse a pre-existing project that has many of these cases.

        foreach (array[i, j]) begin
            array[i][j] = i * j;
        end
MikePopoloski commented 2 years ago

I think this is technically not legal syntax according to the SystemVerilog grammar, though all the major tools seem to support it so slang should as well. Questa gives this warning which I think is fair for slang to do too:

# ** Warning: testbench.sv(8): (vopt-2897) Using non-standard foreach loop variable list syntax.
jrudess commented 2 years ago

The LRM is indeed vague when it comes to MDAs. Consider the following case:

module top;
    int array[8][8];
    initial begin
        foreach (array[3][j]) begin
             array[3][j] = 3 * j;
        end
    end
endmodule

The first dimension is indexed with a hard-coded value, and the intention is to only loop through the 2nd dimension. This gives the same error. But if it's being changed to a warning, then that is probably workable. I guess the only way to code it without a warning would be as follows?

        foreach (array[i, j]) begin
            if (i == 3) begin
                 array[i][j] = i * j;
            end
        end
MikePopoloski commented 2 years ago

Yeah, I was looking at the grammar itself:

foreach ( ps_or_hierarchical_array_identifier [ loop_variables ] ) constraint_set

Where a ps_or_hierarchical_array_identifier can't have any kind of runtime selector after it, it must syntactically end in an identifier.

Note that loop variables can be ommitted to not iterate over that dimension, so I believe your example with a fixed index could be written more succinctly as:

foreach (array[,j]) begin
    array[3][j] = 3 * j;
end

But again, I'm not opposed to supporting this for compatibility sake, especially since all three major vendors support it as well.

jrudess commented 2 years ago

Ahh, thanks! I completely missed that it's legal to omit indices that way.

I'm just trying to pick your brain a bit, since I'm not so familiar with reading BNF notation. Since the variable is an array of an array, I was assuming that array[3] would be considered to be a valid ps_or_hierarchical_array_identifier.

MikePopoloski commented 2 years ago

No, the expansion is given as:

ps_or_hierarchical_array_identifier ::= [ implicit_class_handle . | class_scope | package_scope ] hierarchical_array_identifier
hierarchical_array_identifier ::= hierarchical_identifier
hierarchical_identifier ::= [ $root . ] { identifier constant_bit_select . } identifier

In BNF notation above the square brackets denote optional things and the braces denote a list of 0 or more things.

So essentially an optional $root. followed by 0 or more identifiers and constant bit selects (i.e. compile time constant) and then ending in an identifier. That is, it's a hierarchical path that is known at compile time / elaboration time. Once you hit a variable everything after that is no longer part of the hierarchical name, it's a non-constant bit select or part select or whatever.

jrudess commented 2 years ago

I think I can see why all three vendors are supporting this even though it violates the grammar. I'm starting to think this is a case that should get sent to IEEE to enhance the standard to align with the real-world implementations.

For example, when using dynamically-sized array, omitting an index isn't allowed.

module top;
    int array[][];
    initial begin
        array = new[8];
        foreach (array[i]) begin
            array[i] = new[8];
        end
        foreach (array[,j]) begin
            array[3][j] = 3 * j;
        end
    end
endmodule
slangtest.sv:8:25: error: foreach index 'j' can't iterate over a dynamically sized dimension since a previous dimension was omitted
        foreach (array[,j]) begin
                        ^
MikePopoloski commented 2 years ago

Yeah, there's no way that could work because... what is the size of the dimension it's iterating over? It could depend on which previous dimension you're actually indexing. There isn't a good way to support that without allowing the extended syntax.

MikePopoloski commented 2 years ago

Fixed in d155fd672b487a2f763152950fdf60147a1f2e22