Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Index range of array aggregate with others choice #641

Open
amb5l opened this issue Mar 6, 2023 · 15 comments
Open

Index range of array aggregate with others choice #641

amb5l opened this issue Mar 6, 2023 · 15 comments

Comments

@amb5l
Copy link
Contributor

amb5l commented Mar 6, 2023

I am assigning an unconstrained std_logic_vector signal in a procedure: q <= (others => '0');

NVC does not like this:
Error: index range of array aggregate with others choice cannot be determined from the context

Xilinx XSim is happy with this code, and I think GHDL is too (although it fails for other reasons).

Here's an MCVE, which you can try with nvc --std=2008 -a --relaxed mcve.vhd:

library ieee;
  use ieee.std_logic_1164.all;

entity mcve is
  port(
    clk : in  std_logic;
    rst : in  std_logic;
    d   : in  std_logic_vector(31 downto 0);
    q   : out std_logic_vector(31 downto 0)
  );
end entity mcve;

architecture arch of mcve is

  procedure reg_bus(
    signal clk : in  std_logic;
    signal rst : in  std_logic;
    signal d   : in  std_logic_vector;
    signal q   : out std_logic_vector
  ) is
  begin
    if rising_edge(clk) then
      if rst = '1' then
        q <= (others => '0');
      else
        q <= d;
      end if;
    end if;
  end procedure reg_bus;

begin

  reg_bus(clk,rst,d,q);

end architecture arch;
@tmeissner
Copy link

Mhm, I have similar code and if I remember correctly had to change it. Questa or Synplify (I don't remember which one) didn't compiled it with a similar error.

q <= (others => '0');

changed to

q <= (q'range => '0');

Maybe a look into relevant sections of the LRM could help.

@amb5l
Copy link
Contributor Author

amb5l commented Mar 6, 2023

@tmeissner yours is probably a better coding style - but NVC differs from other compilers in this minor respect. Questa is OK with this assignment in my testing.

@tmeissner
Copy link

tmeissner commented Mar 6, 2023

@amb5l Ah, okay. Maybe my problematic code was in a different context.

I found LCS2016_072b which was incorporated in VHDL-19 standard. It adds functionality that functions now know the returning vector size. So, I assume that code like yours would be invalid in VHDL standards before 2019 if used within a function.

However, procedures behavior isn't mentioned in the LCS.

@amb5l
Copy link
Contributor Author

amb5l commented Mar 6, 2023

Changing others to q'range fixes the error (thanks for this idea) - but this does suggest that NVC "knows" the range in the context of the procedure and could apply it to others.

@tmeissner
Copy link

tmeissner commented Mar 6, 2023

Changing others to q'range fixes the error (thanks for this idea) - but this does suggest that NVC "knows" the range in the context of the procedure and could apply it to others.

Yep, you're right. Maybe @JimLewis can help?

I've looked where I had to use such a coding style and found it. I often use initializer functions for records, which often contain unconstrained vector elements.

type t_axi4_m2s is record
  VALID : std_logic;
  WDATA : std_logic_vector;
  ...
end record;

In the initializer function, I cannot use others, I have to use 'range or something similar.

function initAxi4M2S return t_axi4_m2s is
begin
  return (
    VALID => '0',
    WDATA => (WDATA'range => '0'),
    ...
  );
end function

@nickg
Copy link
Owner

nickg commented Mar 6, 2023

Section 9.3 of the 2008 LRM says "The index range of an array aggregate that has an others choice shall be determinable from the context". It then goes on to list various different contexts, the only one of which I think is applicable here is:

e) As the value expression in an assignment statement, where the target is a declared object (or member thereof), and either the subtype of the target is a fully constrained array subtype or the target is a slice name.

But the target here is unconstrained.

@amb5l
Copy link
Contributor Author

amb5l commented Mar 6, 2023

But the target here is unconstrained.

I've been looking at how target is defined in this situation. Section 11.4 Note 1 says:

Concurrent procedure call statements make it possible to declare procedures representing commonly used processes and to create such processes easily by merely calling the procedure as a concurrent statement.

It seems to me that if this is intention, then the compiler should treat the (in this case fully constrained) actual as the target, rather than the formal parameter.

Set against this we have the note at the end of section 4.2.2.1:

NOTE—Attributes of an actual are never passed into a subprogram. References to an attribute of a formal parameter are
legal only if that formal has such an attribute. Such references retrieve the value of the attribute associated with the
formal.

This implies that the subprogram should be blind to attributes (like range) of the actual (the fully constrained signal).

So I'm left thinking that NVC is LRM compliant and the other tools are relaxed.

@amb5l
Copy link
Contributor Author

amb5l commented Mar 6, 2023

Correction. Section 4.2.2.3 says:

For a signal parameter of mode in or inout, the actual signal is associated with the corresponding formal signal parameter at the start of each call. Thereafter, during the execution of the subprogram body, a reference to the formal signal parameter within an expression is equivalent to a reference to the actual signal.

I think this tips the balance back towards the behaviour I see with GHDL, ModelSim etc.

@nickg
Copy link
Owner

nickg commented Mar 6, 2023

Does GHDL really not produce an error for this (I can't test at that moment)? See ghdl/ghdl#191 and also the proposal to relax this in -2019 which wasn't accepted: http://www.eda-twiki.org/cgi-bin/view.cgi/P1076/RelaxedOthersAggregate

@amb5l
Copy link
Contributor Author

amb5l commented Mar 6, 2023

Does GHDL really not produce an error for this

It does not. The command line is: ghdl -a --std=08 -frelaxed -Wno-hide mcve.vhd

@nickg
Copy link
Owner

nickg commented Mar 6, 2023

How about if you remove -frelaxed?

@amb5l
Copy link
Contributor Author

amb5l commented Mar 6, 2023

You're absolutely right: 'others' choice not allowed for an aggregate in this context. Apologies for overlooking this.

What do you think about section 4.2.2.3... "a reference to the formal signal parameter within an expression is equivalent to a reference to the actual signal"... How can they be equivalent if the the unconstrained-ness of the formal trumps the constrained-ness of the actual? EDIT: I guess the word "expression" is key here.

@nickg
Copy link
Owner

nickg commented Mar 6, 2023

EDIT: I guess the word "expression" is key here.

Yes, there's no expression involving q being evaluated here.

I guess it should be possible to follow GHDL's behaviour and allow this with --relaxed.

@amb5l
Copy link
Contributor Author

amb5l commented Mar 6, 2023

I guess it should be possible to follow GHDL's behaviour and allow this with --relaxed.

Would be good to add that to the wish list. With that said please feel free to close this one.

@JimLewis
Copy link

JimLewis commented Mar 6, 2023

@tmeissner My experience seems to concur with @nickg interpretation. OTOH, it seems like an odd that others is not allowed, but name'range fixes the issue.

With 'range, it seems like we are saying, make the expression the exact size of the elaborated port/parameter - because after elaboration, they are no longer unconstrained. So it makes me wonder why when the aggregate uses others that we are not using the elaborated port sizes rather than considering them unconstrained - maybe Nick's understanding of the compiler/simulator would help me understand why it should be this way.

I have also wondered why we don't have an all for this context when we to fill the entire array - if the language added all here, maybe then it could match the elaborated size of the object and make it so we could be lazier than using 'range.

That said, would I personally write such a change - probably not. Would I support a well written change for this - probably - but a sensible argument from Nick or Tristan could convince me otherwise.

One thing we learned when doing 2008 was that some restrictions were added in the original language because they expected them to be needed, but it turned out that the restrictions were not necessary - in particular the case statement expression needing to have a locally static type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants