ghdl / ghdl

VHDL 2008/93/87 simulator
GNU General Public License v2.0
2.39k stars 363 forks source link

IIR_KIND_CONCATENATION_OPERATOR thrown when instantiating an entity with unconstrained array type #2688

Open n8tlarsen opened 4 months ago

n8tlarsen commented 4 months ago

This issue was present in 3.0.0 and I just installed version 4.1.0 and this issue still exists in my case. Here's the entity:

library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;

entity skid_buffer_in is
    port (
        aclk    : in  std_logic;
        aresetn : in  std_logic;
        -- registered
        r_valid : in  std_logic;
        r_ready : out std_logic;
        r_data  : in  std_logic_vector;
        -- combinational
        c_valid : out std_logic;
        c_ready : in  std_logic;
        c_data  : out std_logic_vector
    );
end entity skid_buffer_in;

architecture rtl of skid_buffer_in is
    constant data_width : natural := r_data'length;
    signal b_data  : std_logic_vector(r_data'range);
    signal b_valid : std_logic; 
begin

    skid : process(aclk, aresetn)
    begin
        if (aresetn = '0') then
            r_ready <= '0';
            b_data  <= (others => '0');
            b_valid <= '0';
        elsif rising_edge(aclk) then
            if c_ready then
                b_valid <= '0';
            end if;
            if (r_valid and r_ready) = '1' then
                if c_ready = '0' then
                    b_data <= r_data;
                end if;
                b_valid <= not c_ready;
                r_ready <= c_ready;
            else
                r_ready <= c_ready or not b_valid;
            end if;
        end if;
    end process skid;

    with to_bit(b_valid) select c_data <= 
        r_data when '0',
        b_data when '1';
    with to_bit(b_valid) select c_valid <= 
        r_valid and r_ready when '0',
        '1' when '1';

end architecture rtl;

and an example instantiating entity (please excuse syntax errors as this is an excerpt from larger file):

library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;

entity inst is
    port (
        aclk    : in  std_logic;
        aresetn : in  std_logic;
        wvalid  : in  std_logic;
        wready  : out std_logic;
        wdata   : in  std_logic_vector(31 downto 0);
        wstrb   : in  std_logic_vector( 3 downto 0)
    );
end entity inst;

architecture struct of inst is
    constant data_bytes : natural := 4;
    signal bi_wvalid, bi_wready   : std_logic;
    signal bi_wdata : std_logic_vector(data_bytes*8-1 downto 0);
    signal bi_wstrb : std_logic_vector(data_bytes-1   downto 0);
begin

    w_skid : entity work.skid_buffer_in
    port map(
        aclk    => aclk,
        aresetn => aresetn,
        r_valid => wvalid,
        r_ready => wready,
        r_data  => wstrb & wdata,
        c_valid => bi_wvalid,
        c_ready => bi_wready,
        c_data(wdata'length+wstrb'high downto wdata'length) => bi_wstrb,
        c_data(wdata'high downto 0) => bi_wdata
    );

end architecture struct;

Surprisingly the following subsitution does not exhibit the internal error, but instead the simulation error: "slice direction doesn't match index direction"

r_data(wdata'length+wstrb'high downto 0)  => wstrb & wdata,

I tested with mcode and llvm backends. The mcode backend throws the error when starting simulation while llvm throws the error at compile time.

Originally posted by @n8tlarsen in https://github.com/ghdl/ghdl/issues/2190#issuecomment-2215571585

tgingold commented 4 months ago

I think your association is not valid. According to LRM08 6.5.6.3 Port clauses:

If the actual part of a given association element for a formal port of a block [...] is an expression that is not globally static, then the given association element
is equivalent to association of the port with an anonymous signal implicitly declared in the declarative
region that immediately encloses the block. The signal has the same subtype as the formal port and is the
target of an implicit concurrent signal assignment statement of the form
anonymous <= E;

As your formal port is not constrained, the signal is declared with an unconstrained subtype, which is not valid.

n8tlarsen commented 4 months ago

I believe the concatenation shown in the example is globally static according to LRM08 9.4.3 Globally static primaries:

An expression is said to be globally static if and only if every operator in the expression denotes
a pure function and every primary in the expression is a globally static primary, where a globally static primary
is a primary that, if it denotes an object or a function, does not denote a dynamically elaborated named entity 
(see 14.6) and is one of the following:
[...]
g) An array aggregate, if and only if 
   1) All expressions in its element associations are globally static expressions, and 
   2) All ranges in its element associations are globally static ranges
[...]
i) A function call whose function name denotes a pure function and whose actual parameters are each 
   globally static expressions

After reading through these sections I tried the functional form of the concatenation operator and found that after circumventing a different error it compiles and simulates just fine:

architecture struct of inst is
    constant data_bytes : natural := 4;
    signal bi_wvalid, bi_wready   : std_logic;
    signal bi_wdata : std_logic_vector(data_bytes*8-1 downto 0);
    signal bi_wstrb : std_logic_vector(data_bytes-1   downto 0);
    signal bi_wblob : std_logic_vector(wdata'length+wstrb'length-1 downto 0);
begin

    w_skid : entity work.skid_buffer_in
    port map(
        aclk    => aclk,
        aresetn => aresetn,
        r_valid => wvalid,
        r_ready => wready,
        r_data  => "&"(wstrb, wdata),
        c_valid => bi_wvalid,
        c_ready => bi_wready,
        c_data  => bi_wblob -- changed to a single output, GHDL complained about mismatched slice directions 
    );
    bi_wstrb <= bi_wblob(wdata'length+wstrb'high downto wdata'length);
    bi_wdata <= bi_wblob(wdata'high downto 0);

end architecture struct;
tgingold commented 4 months ago

I am confused.

First, the concatenation cannot be static as a signal is never static.

Second, even if the function call is used, it failed on ghdl. Which version are you using ?

n8tlarsen commented 4 months ago

I'm using version 4.1.0 compiled from the release sources with the LLVM backend.

I'm second-guessing my interpretation of globally static. However, there is still something strange going on here. The following concatenation of constants (which should definitely be globally static) in the port actual fails with the same error.

library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;

entity inst is
    port (
        aclk    : in  std_logic;
        aresetn : in  std_logic;
        wvalid  : in  std_logic;
        wready  : out std_logic;
        wdata   : in  std_logic_vector(31 downto 0);
        wstrb   : in  std_logic_vector( 3 downto 0)
    );
end entity inst;

architecture struct of inst is
    constant data_bytes : natural := 4;
    signal bi_wvalid, bi_wready   : std_logic;
    signal bi_wdata : std_logic_vector(data_bytes*8-1 downto 0);
    signal bi_wstrb : std_logic_vector(data_bytes-1   downto 0);
    signal bi_wblob : std_logic_vector(wdata'length+wstrb'length-1 downto 0);
    constant left : std_logic_vector(data_bytes-1   downto 0) := (others => '1');
    constant right : std_logic_vector(data_bytes*8-1 downto 0) := (others => '1');
begin

    w_skid : entity work.skid_buffer_in
    port map(
        aclk    => aclk,
        aresetn => aresetn,
        r_valid => wvalid,
        r_ready => wready,
        r_data  => left & right,
        c_valid => bi_wvalid,
        c_ready => bi_wready,
        c_data  => bi_wblob -- changed to a single output, GHDL complained about mismatched slice directions 
    );
    bi_wstrb <= bi_wblob(wdata'length+wstrb'high downto wdata'length);
    bi_wdata <= bi_wblob(wdata'high downto 0);

end architecture struct;
n8tlarsen commented 4 months ago

Another data point is replacing the concatenation expression with a signal whose concurrent assignment is the same expression does not cause the error. If signals are not globally static, then this should also be illegal, but GHDL accepts it:

library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;

entity inst is
    port (
        aclk    : in  std_logic;
        aresetn : in  std_logic;
        wvalid  : in  std_logic;
        wready  : out std_logic;
        wdata   : in  std_logic_vector(31 downto 0);
        wstrb   : in  std_logic_vector( 3 downto 0)
    );
end entity inst;

architecture struct of inst is
    constant data_bytes : natural := 4;
    signal bi_wvalid, bi_wready   : std_logic;
    signal bi_wdata : std_logic_vector(data_bytes*8-1 downto 0);
    signal bi_wstrb : std_logic_vector(data_bytes-1   downto 0);
    signal wblob : std_logic_vector(wdata'length+wstrb'length-1 downto 0);
    signal bi_wblob : std_logic_vector(wdata'length+wstrb'length-1 downto 0);
begin

    w_skid : entity work.skid_buffer_in
    port map(
        aclk    => aclk,
        aresetn => aresetn,
        r_valid => wvalid,
        r_ready => wready,
        r_data  => wblob,
        c_valid => bi_wvalid,
        c_ready => bi_wready,
        c_data  => bi_wblob 
    );
    wblob <= wstrb & wdata;
    bi_wstrb <= bi_wblob(wdata'length+wstrb'high downto wdata'length);
    bi_wdata <= bi_wblob(wdata'high downto 0);

end architecture struct;
tgingold commented 4 months ago

I'm using version 4.1.0 compiled from the release sources with the LLVM backend.

You should try with the mcode backend, as it handles much better this construct.

I'm second-guessing my interpretation of globally static. However, there is still something strange going on here. The following concatenation of constants (which should definitely be globally static) in the port actual fails with the same error.

This one is handled correctly by the mcode backend.

tgingold commented 4 months ago

Another data point is replacing the concatenation expression with a signal whose concurrent assignment is the same expression does not cause the error. If signals are not globally static, then this should also be illegal, but GHDL accepts it:

Why should it be illegal ? It's a very different case as you are associating a port with a signal, and not with an expression.

n8tlarsen commented 4 months ago

I think I'm understanding correctly now. Signals are separate from expressions, though expressions can contain signals and when they do, they are not globally static. The excerpt you mentioned from 6.5.6.3 Port clauses explicitly calls out expressions and makes no mention of signals.

In the original example, what I'm trying to do violates the LRM. That being said, I do think the error message should be improved to reflect that fact. I noticed similar messages for other operators including and, or, not. Other operators probably have a similar issue but I didn't test them.

tgingold commented 4 months ago

Yes, sure I need to fix the crash.