[PATCH 0/2] tricore: IMASK/EXTR corner case fixes

[PATCH 0/2] tricore: IMASK/EXTR corner case fixes

[Bastian Koppelmann]

Hi,

while testing Andreas' patch [1], I found two more bugs in IMASK and EXTR which
I fixed here.

[1] https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg03916.html

Cheers,
Bastian

Bastian Koppelmann (2):
  target/tricore: Fix imask OPC2_32_RRPW_IMASK for r3+1 == r2
  target/tricore: Fix OPC2_32_RRPW_EXTR for width=0

 target/tricore/translate.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

-- 
2.30.1

[PATCH 1/2] target/tricore: Fix imask OPC2_32_RRPW_IMASK for r3+1 == r2

[Bastian Koppelmann]

if r3+1 and r2 are the same then we would overwrite r2 with our first
move and use the wrong result for the shift. Thus we store the result
from the mov in a temp.

Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
---
 target/tricore/translate.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index ebeddf8f4a..67a7f646a2 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -6989,6 +6989,7 @@ static void decode_rrpw_extract_insert(DisasContext *ctx)
     uint32_t op2;
     int r1, r2, r3;
     int32_t pos, width;
+    TCGv temp;
 
     op2 = MASK_OP_RRPW_OP2(ctx->opcode);
     r1 = MASK_OP_RRPW_S1(ctx->opcode);
@@ -7021,10 +7022,15 @@ static void decode_rrpw_extract_insert(DisasContext *ctx)
         break;
     case OPC2_32_RRPW_IMASK:
         CHECK_REG_PAIR(r3);
+        temp = tcg_temp_new();
+
         if (pos + width <= 32) {
-            tcg_gen_movi_tl(cpu_gpr_d[r3+1], ((1u << width) - 1) << pos);
+            tcg_gen_movi_tl(temp, ((1u << width) - 1) << pos);
             tcg_gen_shli_tl(cpu_gpr_d[r3], cpu_gpr_d[r2], pos);
+            tcg_gen_mov_tl(cpu_gpr_d[r3+1], temp);
         }
+
+        tcg_temp_free(temp);
         break;
     case OPC2_32_RRPW_INSERT:
         if (pos + width <= 32) {
-- 
2.30.1

Re: [PATCH 1/2] target/tricore: Fix imask OPC2_32_RRPW_IMASK for r3+1 == r2

[Richard Henderson]

On 3/5/21 5:26 AM, Bastian Koppelmann wrote:
> @@ -6989,6 +6989,7 @@ static void decode_rrpw_extract_insert(DisasContext *ctx)
>       uint32_t op2;
>       int r1, r2, r3;
>       int32_t pos, width;
> +    TCGv temp;
>   
>       op2 = MASK_OP_RRPW_OP2(ctx->opcode);
>       r1 = MASK_OP_RRPW_S1(ctx->opcode);
> @@ -7021,10 +7022,15 @@ static void decode_rrpw_extract_insert(DisasContext *ctx)
>           break;
>       case OPC2_32_RRPW_IMASK:
>           CHECK_REG_PAIR(r3);
> +        temp = tcg_temp_new();
> +
>           if (pos + width <= 32) {
> -            tcg_gen_movi_tl(cpu_gpr_d[r3+1], ((1u << width) - 1) << pos);
> +            tcg_gen_movi_tl(temp, ((1u << width) - 1) << pos);
>               tcg_gen_shli_tl(cpu_gpr_d[r3], cpu_gpr_d[r2], pos);
> +            tcg_gen_mov_tl(cpu_gpr_d[r3+1], temp);
>           }
> +
> +        tcg_temp_free(temp);

You could restrict the variable to the if block.

Either way,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

[PATCH 2/2] target/tricore: Fix OPC2_32_RRPW_EXTR for width=0

[Bastian Koppelmann]

if width was 0 we would run into the assertion:

qemu-system-tricore: ../upstream/tcg/tcg-op.c:217: tcg_gen_sari_i32: Assertion `arg2 >= 0 && arg2 < 32' failed.o

The instruction manuel specifies undefined behaviour for this case. So
we bring this in line with the golden Infineon simlator 'tsim', which
simply writes 0 to the result in case of width=0.

Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
---
 target/tricore/translate.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index 67a7f646a2..d8b773ab37 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -6998,10 +6998,16 @@ static void decode_rrpw_extract_insert(DisasContext *ctx)
     pos = MASK_OP_RRPW_POS(ctx->opcode);
     width = MASK_OP_RRPW_WIDTH(ctx->opcode);
 
+
     switch (op2) {
     case OPC2_32_RRPW_EXTR:
+        if (width == 0) {
+                tcg_gen_movi_tl(cpu_gpr_d[r3], 0);
+                break;
+        }
+
         if (pos + width <= 32) {
-            /* optimize special cases */
+                        /* optimize special cases */
             if ((pos == 0) && (width == 8)) {
                 tcg_gen_ext8s_tl(cpu_gpr_d[r3], cpu_gpr_d[r1]);
             } else if ((pos == 0) && (width == 16)) {
-- 
2.30.1

Re: [PATCH 2/2] target/tricore: Fix OPC2_32_RRPW_EXTR for width=0

[Philippe Mathieu-Daudé]

On 3/5/21 2:26 PM, Bastian Koppelmann wrote:
> if width was 0 we would run into the assertion:
> 
> qemu-system-tricore: ../upstream/tcg/tcg-op.c:217: tcg_gen_sari_i32: Assertion `arg2 >= 0 && arg2 < 32' failed.o

Maybe strip "../upstream/"

> 
> The instruction manuel specifies undefined behaviour for this case. So

"manual"

> we bring this in line with the golden Infineon simlator 'tsim', which
> simply writes 0 to the result in case of width=0.
> 
> Signed-off-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>
> ---
>  target/tricore/translate.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/target/tricore/translate.c b/target/tricore/translate.c
> index 67a7f646a2..d8b773ab37 100644
> --- a/target/tricore/translate.c
> +++ b/target/tricore/translate.c
> @@ -6998,10 +6998,16 @@ static void decode_rrpw_extract_insert(DisasContext *ctx)
>      pos = MASK_OP_RRPW_POS(ctx->opcode);
>      width = MASK_OP_RRPW_WIDTH(ctx->opcode);
>  
> +

Spurious change?

>      switch (op2) {
>      case OPC2_32_RRPW_EXTR:
> +        if (width == 0) {
> +                tcg_gen_movi_tl(cpu_gpr_d[r3], 0);
> +                break;
> +        }
> +
>          if (pos + width <= 32) {
> -            /* optimize special cases */
> +                        /* optimize special cases */

Spurious change?

>              if ((pos == 0) && (width == 8)) {
>                  tcg_gen_ext8s_tl(cpu_gpr_d[r3], cpu_gpr_d[r1]);
>              } else if ((pos == 0) && (width == 16)) {
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 2/2] target/tricore: Fix OPC2_32_RRPW_EXTR for width=0

[Richard Henderson]

On 3/5/21 5:26 AM, Bastian Koppelmann wrote:
>       case OPC2_32_RRPW_EXTR:
> +        if (width == 0) {
> +                tcg_gen_movi_tl(cpu_gpr_d[r3], 0);
> +                break;
> +        }
> +
>           if (pos + width <= 32) {
> -            /* optimize special cases */
> +                        /* optimize special cases */
>               if ((pos == 0) && (width == 8)) {

Indentation is definitely off, both with the unrelated comment change Phil 
pointed out, and in the new code block.

Otherwise,
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~