-
Notifications
You must be signed in to change notification settings - Fork 258
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
Questa Sim 2022.4_2 changes default bit ordering in unconstrained arrays. Test tb_uart_lib.tb_uart_rx.test_receives_one_byte broken. #889
Comments
The first thing Flipping the order of the loop may fix things for your version of Questa but it will fail on all other versions and simulators |
Indeed, I just observed the flip in the uart_master VC component output. I've run the whole vhdl test suite (vunit/examples/vhdl/docker_runall.sh) and no other errors showed up. Not what I would expect if the flip happens elsewhere.
Using get_simulator_name() from the py class could be a way to get around it. I wonder if this new questasim feature violates the LRM: then it's a bug. |
I added a swap_bit_order argument to the uart_master: |
Ignore anything previously said and have a look at the following: Hard to believe. |
I also had a look at this and agree that it is a bug in Questa. It "optimizes" the code in a way that effectively skips the normalization step to
Do you have a support account with Siemens? If so, I suggest you create a minimal example of this problem and file a bug. |
I will do that and report here. |
It seems to still be the same behaviour in Questa 2023.4. |
@nicdes Any word from Siemens? |
I just opened a support case with Siemens with the following minimal example that also reproduces the error. (The root of the problem seems to lie in their procedure/function optimization routines as it works fine if you normalize "directly" in the process.). I'll keep you posted if I hear anything back. library ieee;
use ieee.std_logic_1164.all;
entity foo is
end entity foo;
architecture a of foo is
begin
test: process is
procedure print_vector(constant c: std_logic_vector) is
begin
for i in c'range loop
report integer'image(i) & ": " & std_logic'image(c(i));
end loop;
end procedure;
procedure print_normalized_vector(constant c: std_logic_vector) is
constant v : std_logic_vector(c'length - 1 downto 0) := c;
begin
print_vector(v);
end procedure;
constant slv : std_logic_vector := x"80";
constant slv_normalized : std_logic_vector(slv'length - 1 downto 0) := slv;
begin
print_normalized_vector(slv);
report "";
print_vector(slv_normalized);
wait;
end process;
end architecture;
-- nvc -a foo.vhd -e foo -r
-- vcom +acc foo.vhd && vsim -voptargs="+acc" -c -do "run -all; exit" 'work.foo(a)' In Questa 2023.4, this evaluates to
even though both vectors should be the exact same. |
So Siemens just confirmed the bug, and they will look into fixing it for future versions of Questa. Until then, they found the following workaround of just replacing the diff --git a/vunit/vhdl/verification_components/src/stream_master_pkg-body.vhd b/vunit/vhdl/verification_components/src/stream_master_pkg-body.vhd
index 708596d5..1270f65c 100644
--- a/vunit/vhdl/verification_components/src/stream_master_pkg-body.vhd
+++ b/vunit/vhdl/verification_components/src/stream_master_pkg-body.vhd
@@ -21,7 +21,7 @@ package body stream_master_pkg is
data : std_logic_vector;
last : boolean := false) is
variable msg : msg_t := new_msg(stream_push_msg);
- constant normalized_data : std_logic_vector(data'length-1 downto 0) := data;
+ variable normalized_data : std_logic_vector(data'length-1 downto 0) := data;
begin
push_std_ulogic_vector(msg, normalized_data);
push_boolean(msg, last); |
@m42uko Good that they confirmed the bug and that the workaround we also found works in general. I will change this specific instance such that the example works for the affected simulator versions. There are probably other VUnit APIs that would fail for the same reason if the input is a literal. It's not reasonable to find and fix all of those so we will simply have to remember this issue and recommend people having problems to assign their literals to intermediate variables before calling the API. |
Updating questasim from 2020.1_1 to 2022.4_2 breakes the test 'tb_uart_lib.tb_uart_rx.test_receives_one_byte':
And I expect many others. Note that the got value is the swapped version of the expected value.
The reason is that this procedure
vunit/examples/vhdl/uart/src/test/tb_uart_rx.vhd
Line 54 in 868eea1
ends up assigning the value x"77" to the unconstrained data port in the uart_master verification component:
vunit/vunit/vhdl/verification_components/src/uart_master.vhd
Line 29 in 868eea1
Questa Sim has changed the bit ordering of expressions like x"77" when assigned to unconstrained std_logic_vector ports.
My fix is replacing
vunit/vunit/vhdl/verification_components/src/uart_master.vhd
Line 43 in 868eea1
with:
VUnit version: 4.6.0
Simulator: Questa Sim-64 vsim 2022.4_2 Simulator 2022.12 Dec 9 2022
The text was updated successfully, but these errors were encountered: