From b83ebf3c8ecaf00c41359fe2ecef21af29864494 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Sat, 1 Jan 2022 10:46:56 +0100 Subject: [PATCH 1/4] tgt-vvp: Fix store skip on real typed array entries When assigning a value to a real typed array entry the vvp `%store/reala` instruction. This instruction will skip the store if the vvp flag 4 is set. This is to handle the case where the index is undefined. When the index into the array is an immediate value the flag 4 is not cleared. If a previous instruction set flag 4 the store will be skipped. E.g. for the following r[0] will remain 0.0 since the assignment to it is skipped. ``` integer a = 0; real r[1:0]; if (a == 0) begin r[0] = 1.23; end ``` Fix this by using the `draw_eval_expr_into_integer()` helper function to evaluate the index into a word register. The function correctly handles all the special cases. Signed-off-by: Lars-Peter Clausen --- tgt-vvp/stmt_assign.c | 27 ++------------------------- 1 file changed, 2 insertions(+), 25 deletions(-) diff --git a/tgt-vvp/stmt_assign.c b/tgt-vvp/stmt_assign.c index ac604db73..65ccc831c 100644 --- a/tgt-vvp/stmt_assign.c +++ b/tgt-vvp/stmt_assign.c @@ -759,31 +759,8 @@ static void store_real_to_lval(ivl_lval_t lval) ivl_expr_t word_ex = ivl_lval_idx(lval); int word_ix = allocate_word(); - /* If the word index is a constant, then we can write - directly to the word and save the index calculation. - Out-of-bounds and undefined indices are converted to - a canonical index of 'bx during elaboration, and we - don't try to optimise that case. */ - if (word_ex && number_is_immediate(word_ex, IMM_WID, 0) && - !number_is_unknown(word_ex)) { - unsigned long use_word = get_number_immediate(word_ex); - assert(use_word < ivl_signal_array_count(var)); - fprintf(vvp_out, " %%ix/load %d, %lu, 0;\n", - word_ix, use_word); - fprintf(vvp_out, " %%store/reala v%p, %d;\n", - var, word_ix); - - } else { - unsigned do_store = transient_id++; - unsigned end_store = transient_id++; - draw_eval_expr_into_integer(word_ex, word_ix); - fprintf(vvp_out, " %%jmp/0 t_%u, 4;\n", do_store); - fprintf(vvp_out, " %%pop/real 1;\n"); - fprintf(vvp_out, " %%jmp t_%u;\n", end_store); - fprintf(vvp_out, "t_%u ;\n", do_store); - fprintf(vvp_out, " %%store/reala v%p, %d;\n", var, word_ix); - fprintf(vvp_out, "t_%u ;\n", end_store); - } + draw_eval_expr_into_integer(word_ex, word_ix); + fprintf(vvp_out, " %%store/reala v%p, %d;\n", var, word_ix); clr_word(word_ix); } From 2abfef68ff53e45f4110c4db90f8a8c7467b127e Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Sat, 1 Jan 2022 11:00:15 +0100 Subject: [PATCH 2/4] tgt-vvp: Fix load skip for assignment operator to array entry The vvp `%load/vec4a` instruction will skip the load if vvp flag 4 is set and return 'x. This is meant for handling the case where the index is undefined. For assignment operators on array entries, when the index is an immediate value, vvp flag 4 is not cleared before the load instruction. If a previous instruction set flag 4 it load yield 'x. E.g. for the following sequence `x[0]` should be `11`, but will be `'x`. ``` integer x[10]; logic a = 1'b0; x[0] = 10; if (a == 0) begin x[0] += 1; end ``` Properly clear the flag before the load instruction to handle this correctly. Signed-off-by: Lars-Peter Clausen --- tgt-vvp/stmt_assign.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tgt-vvp/stmt_assign.c b/tgt-vvp/stmt_assign.c index 65ccc831c..754722646 100644 --- a/tgt-vvp/stmt_assign.c +++ b/tgt-vvp/stmt_assign.c @@ -161,6 +161,7 @@ static void get_vec_from_lval_slice(ivl_lval_t lval, struct vec_slice_info*slice if (use_word < ivl_signal_array_count(sig)) { fprintf(vvp_out, " %%ix/load 3, %lu, 0;\n", use_word); + fprintf(vvp_out, " %%flag_set/imm 4, 0;\n"); fprintf(vvp_out, " %%load/vec4a v%p, 3;\n", sig); } else { assert(wid <= 32); From e263d5b2687c44eab3b50b7a43fdc637a23ac9d6 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Sat, 1 Jan 2022 10:45:52 +0100 Subject: [PATCH 3/4] tgt-vvp: Fix store skip for assignment operator on dynamic part select The operator assignment on dynamic part selects uses the `%store/vec4` instruction to store the value. This instruction will skip the assignment if flag 4 is set. This is for handling the case where the index is undefined. Since the left hand side of the assignment is an arbitrary expression it can change the flag. The implementation handles this by making a copy of the flag and restoring it before executing the `%store/vec4` instruction. The flag is set by the `%ix/vec4` instruction when loading the index register. But the copy of the flag is made before that and just picks up the flag that was stored by previous expressions. This can cause the store to be skipped when it shouldn't. E.g. in the following code the increment will be skipped. Flag 4 is used from the `a == 0` comparison, rather than from computing the part select index. ``` int a = 0; if (a == 0) begin a[a+:2] += 1; end $display(a); // Will print 0, should print 1 ``` Fix this by moving the copy of the flag after the `%ix/vec4` instruction. Signed-off-by: Lars-Peter Clausen --- tgt-vvp/stmt_assign.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tgt-vvp/stmt_assign.c b/tgt-vvp/stmt_assign.c index 754722646..e240fd619 100644 --- a/tgt-vvp/stmt_assign.c +++ b/tgt-vvp/stmt_assign.c @@ -147,9 +147,9 @@ static void get_vec_from_lval_slice(ivl_lval_t lval, struct vec_slice_info*slice fprintf(vvp_out, " %%load/vec4 v%p_%lu;\n", sig, use_word); draw_eval_vec4(part_off_ex); - fprintf(vvp_out, " %%flag_mov %u, 4;\n", slice->u_.part_select_dynamic.x_flag); fprintf(vvp_out, " %%dup/vec4;\n"); fprintf(vvp_out, " %%ix/vec4 %d;\n", slice->u_.part_select_dynamic.word_idx_reg); + fprintf(vvp_out, " %%flag_mov %u, 4;\n", slice->u_.part_select_dynamic.x_flag); fprintf(vvp_out, " %%part/u %u;\n", wid); } else if (ivl_signal_dimensions(sig) > 0 && word_ix == 0) { From c2c758369d54eac0c2c2693207ea7fa5800b1bc2 Mon Sep 17 00:00:00 2001 From: Lars-Peter Clausen Date: Mon, 16 May 2022 11:00:28 +0200 Subject: [PATCH 4/4] Add regression tests for accidental store/load skip Check that for the following operations the load or store is not skipped after a operation that sets vvp flag 4. * Assignment to immediate indexed real array entry * Assignment operator on immediate indexed vector array entry * Assignment operator on dynamic vector part select Signed-off-by: Lars-Peter Clausen --- ivtest/ivltests/assign_op_after_cmp1.v | 24 ++++++++++++++++++++ ivtest/ivltests/assign_op_after_cmp2.v | 22 ++++++++++++++++++ ivtest/ivltests/real_array_store_after_cmp.v | 23 +++++++++++++++++++ ivtest/regress-sv.list | 2 ++ ivtest/regress-vlg.list | 1 + ivtest/regress-vlog95.list | 1 + 6 files changed, 73 insertions(+) create mode 100644 ivtest/ivltests/assign_op_after_cmp1.v create mode 100644 ivtest/ivltests/assign_op_after_cmp2.v create mode 100644 ivtest/ivltests/real_array_store_after_cmp.v diff --git a/ivtest/ivltests/assign_op_after_cmp1.v b/ivtest/ivltests/assign_op_after_cmp1.v new file mode 100644 index 000000000..a8fab8820 --- /dev/null +++ b/ivtest/ivltests/assign_op_after_cmp1.v @@ -0,0 +1,24 @@ +// Check that a assignment operator on an integer array entry with an immediate +// index works if it happes after a comparison that sets vvp flag 4 to 0. + +module test; + + logic [7:0] x[10]; + logic a = 1'b0; + + initial begin + x[0] = 10; + if (a == 0) begin + // Make sure that this update happens, even though the compare above + // cleared set vvp flag 4 + x[0] += 1; + end + + if (x[0] == 11) begin + $display("PASSED"); + end else begin + $display("FAILED. Expected 11, got %0d", x[0]); + end + end + +endmodule diff --git a/ivtest/ivltests/assign_op_after_cmp2.v b/ivtest/ivltests/assign_op_after_cmp2.v new file mode 100644 index 000000000..f9db0a6ed --- /dev/null +++ b/ivtest/ivltests/assign_op_after_cmp2.v @@ -0,0 +1,22 @@ +// Check that a assignment operator on a dynamic part select of an vector works +// if it happes after a comparison that sets vvp flag 4 to 0. + +module test; + + logic [7:0] a = 8'h0; + + initial begin + if (a == 0) begin + // Make sure that this update happens, even though the compare above + // cleared set vvp flag 4 + a[a+:1] += 1; + end + + if (a == 1) begin + $display("PASSED"); + end else begin + $display("FAILED. Expected 1, got %0d", a); + end + end + +endmodule diff --git a/ivtest/ivltests/real_array_store_after_cmp.v b/ivtest/ivltests/real_array_store_after_cmp.v new file mode 100644 index 000000000..323e9e46d --- /dev/null +++ b/ivtest/ivltests/real_array_store_after_cmp.v @@ -0,0 +1,23 @@ +// Check that a store to am entry of real typed array with an immediate index +// works if it happes after a comparison that sets vvp flag 4 to 0. + +module test; + + integer a = 0; + real r[1:0]; + + initial begin + if (a == 0) begin + // Make sure that this store happens, even though the compare above + // cleared set vvp flag 4 + r[0] = 1.23; + end + + if (r[0] == 1.23) begin + $display("PASSED"); + end else begin + $display("FAILED. Expected %f, got %f", 1.23, r[0]); + end + end +endmodule + diff --git a/ivtest/regress-sv.list b/ivtest/regress-sv.list index 70ae36555..6c3afca56 100644 --- a/ivtest/regress-sv.list +++ b/ivtest/regress-sv.list @@ -96,6 +96,8 @@ array_size normal,-g2005-sv ivltests # test [size] arrays array_string normal,-g2009 ivltests array_unpacked_sysfunct normal,-g2005-sv ivltests array_packed normal,-g2005-sv ivltests +assign_op_after_cmp1 normal,-g2009 ivltests +assign_op_after_cmp2 normal,-g2009 ivltests assign_op_concat normal,-g2009 ivltests assign_op_type normal,-g2009 ivltests bitp1 normal,-g2005-sv ivltests diff --git a/ivtest/regress-vlg.list b/ivtest/regress-vlg.list index 7c019c309..0863a04ff 100644 --- a/ivtest/regress-vlg.list +++ b/ivtest/regress-vlg.list @@ -1463,6 +1463,7 @@ real9 normal ivltests real10 normal ivltests real11 normal ivltests real_array_multi_dim normal ivltests +real_array_store_after_cmp normal ivltests real_assign_deassign normal ivltests real_concat_invalid2 CE ivltests real_delay normal,-gspecify ivltests gold=real_delay.gold diff --git a/ivtest/regress-vlog95.list b/ivtest/regress-vlog95.list index cd6a919e5..bde6fa422 100644 --- a/ivtest/regress-vlog95.list +++ b/ivtest/regress-vlog95.list @@ -248,6 +248,7 @@ pr3592746 CE ivltests real_array CE ivltests real_array_nb CE,-pallowsigned=1 ivltests real_array_multi_dim CE,-pallowsigned=1 ivltests +real_array_store_after_cmp CE ivltests scan-invalid CE ivltests sel_rval_bit_ob CE ivltests sel_rval_part_ob CE ivltests