[Qemu-devel] [PATCH 0/5] tricore: adding new instructions and fixing issues

[Qemu-devel] [PATCH 0/5] tricore: adding new instructions and fixing issues

[David Brenken]

From: David Brenken <david.brenken@efs-auto.de>

Hello everyone,

in the scope of this patchset we added implementations for the following 
instructions for the target TriCore:

QSEED, FTOIZ, UTOF

For the implementation of the QSEED instruction we wrote a test application to 
record the QSEED results on the actual target. We recognized that the results
of the instruction can be clustered in blocks and do not use all available mantissa bits.
After investigating on how to calculate the square root on floats, which can easily
be done using shift and add, we implemented it using a 128 entry LUT and finetuned
the values to exactly match the hardware results.

Furthermore we added a fix for the RRPW_INSERT instruction.

Internally we are now using QEMU head and encountered strange issues during 
execution. Sometimes the PC of the target was set to address 0x0 which was wrong 
behaviour. A detailed analysis (using valgrind and git bisect) resulted in the fix 
to reset the ctx variable before generating intermediate code.

Best regards

David Brenken


Andreas Konopik (1):
  tricore: add QSEED instruction

David Brenken (3):
  tricore: add FTOIZ instruction
  tricore: add UTOF instruction
  tricore: fix RRPW_INSERT instruction

Georg Hofstetter (1):
  tricore: reset DisasContext before generating code

 target/tricore/fpu_helper.c | 129 ++++++++++++++++++++++++++++++++++++
 target/tricore/helper.h     |   3 +
 target/tricore/translate.c  |  21 +++++-
 3 files changed, 150 insertions(+), 3 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/5] tricore: add FTOIZ instruction

[David Brenken]

From: David Brenken <david.brenken@efs-auto.de>

Signed-off-by: Andreas Konopik <andreas.konopik@efs-auto.de>
Signed-off-by: David Brenken <david.brenken@efs-auto.de>
Signed-off-by: Georg Hofstetter <georg.hofstetter@efs-auto.de>
Signed-off-by: Robert Rasche <robert.rasche@efs-auto.de>
Signed-off-by: Lars Biermanski <lars.biermanski@efs-auto.de>

---
 target/tricore/fpu_helper.c | 25 +++++++++++++++++++++++++
 target/tricore/helper.h     |  1 +
 target/tricore/translate.c  |  3 +++
 3 files changed, 29 insertions(+)

diff --git a/target/tricore/fpu_helper.c b/target/tricore/fpu_helper.c
index d8a6c0d25b..f079d9e939 100644
--- a/target/tricore/fpu_helper.c
+++ b/target/tricore/fpu_helper.c
@@ -303,6 +303,31 @@ uint32_t helper_itof(CPUTriCoreState *env, uint32_t arg)
     return (uint32_t)f_result;
 }
 
+uint32_t helper_ftoiz(CPUTriCoreState *env, uint32_t arg)
+{
+    float32 f_arg = make_float32(arg);
+    uint32_t result;
+    int32_t flags;
+
+    result = float32_to_int32_round_to_zero(f_arg, &env->fp_status);
+
+    flags = f_get_excp_flags(env);
+    if (flags & float_flag_invalid) {
+        flags &= ~float_flag_inexact;
+        if (float32_is_any_nan(f_arg)) {
+            result = 0;
+        }
+    }
+
+    if (flags) {
+        f_update_psw_flags(env, flags);
+    } else {
+        env->FPU_FS = 0;
+    }
+
+    return result;
+}
+
 uint32_t helper_ftouz(CPUTriCoreState *env, uint32_t arg)
 {
     float32 f_arg = make_float32(arg);
diff --git a/target/tricore/helper.h b/target/tricore/helper.h
index f60e81096b..16b62edf7f 100644
--- a/target/tricore/helper.h
+++ b/target/tricore/helper.h
@@ -111,6 +111,7 @@ DEF_HELPER_4(fmsub, i32, env, i32, i32, i32)
 DEF_HELPER_3(fcmp, i32, env, i32, i32)
 DEF_HELPER_2(ftoi, i32, env, i32)
 DEF_HELPER_2(itof, i32, env, i32)
+DEF_HELPER_2(ftoiz, i32, env, i32)
 DEF_HELPER_2(ftouz, i32, env, i32)
 DEF_HELPER_2(updfl, void, env, i32)
 /* dvinit */
diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index 352f52bb4a..66cdc63286 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -6764,6 +6764,9 @@ static void decode_rr_divide(CPUTriCoreState *env, DisasContext *ctx)
     case OPC2_32_RR_UPDFL:
         gen_helper_updfl(cpu_env, cpu_gpr_d[r1]);
         break;
+    case OPC2_32_RR_FTOIZ:
+        gen_helper_ftoiz(cpu_gpr_d[r3], cpu_env, cpu_gpr_d[r1]);
+        break;
     default:
         generate_trap(ctx, TRAPC_INSN_ERR, TIN2_IOPC);
     }
-- 
2.17.1

[Qemu-devel] [PATCH 2/5] tricore: add UTOF instruction

[David Brenken]

From: David Brenken <david.brenken@efs-auto.de>

Signed-off-by: Andreas Konopik <andreas.konopik@efs-auto.de>
Signed-off-by: David Brenken <david.brenken@efs-auto.de>
Signed-off-by: Georg Hofstetter <georg.hofstetter@efs-auto.de>
Signed-off-by: Robert Rasche <robert.rasche@efs-auto.de>
Signed-off-by: Lars Biermanski <lars.biermanski@efs-auto.de>

---
 target/tricore/fpu_helper.c | 16 ++++++++++++++++
 target/tricore/helper.h     |  1 +
 target/tricore/translate.c  |  3 +++
 3 files changed, 20 insertions(+)

diff --git a/target/tricore/fpu_helper.c b/target/tricore/fpu_helper.c
index f079d9e939..432079c8e2 100644
--- a/target/tricore/fpu_helper.c
+++ b/target/tricore/fpu_helper.c
@@ -303,6 +303,22 @@ uint32_t helper_itof(CPUTriCoreState *env, uint32_t arg)
     return (uint32_t)f_result;
 }
 
+uint32_t helper_utof(CPUTriCoreState *env, uint32_t arg)
+{
+    float32 f_result;
+    uint32_t flags;
+
+    f_result = uint32_to_float32(arg, &env->fp_status);
+
+    flags = f_get_excp_flags(env);
+    if (flags) {
+        f_update_psw_flags(env, flags);
+    } else {
+        env->FPU_FS = 0;
+    }
+    return (uint32_t)f_result;
+}
+
 uint32_t helper_ftoiz(CPUTriCoreState *env, uint32_t arg)
 {
     float32 f_arg = make_float32(arg);
diff --git a/target/tricore/helper.h b/target/tricore/helper.h
index 16b62edf7f..f1a5cb367e 100644
--- a/target/tricore/helper.h
+++ b/target/tricore/helper.h
@@ -111,6 +111,7 @@ DEF_HELPER_4(fmsub, i32, env, i32, i32, i32)
 DEF_HELPER_3(fcmp, i32, env, i32, i32)
 DEF_HELPER_2(ftoi, i32, env, i32)
 DEF_HELPER_2(itof, i32, env, i32)
+DEF_HELPER_2(utof, i32, env, i32)
 DEF_HELPER_2(ftoiz, i32, env, i32)
 DEF_HELPER_2(ftouz, i32, env, i32)
 DEF_HELPER_2(updfl, void, env, i32)
diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index 66cdc63286..a8b4de647a 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -6764,6 +6764,9 @@ static void decode_rr_divide(CPUTriCoreState *env, DisasContext *ctx)
     case OPC2_32_RR_UPDFL:
         gen_helper_updfl(cpu_env, cpu_gpr_d[r1]);
         break;
+    case OPC2_32_RR_UTOF:
+        gen_helper_utof(cpu_gpr_d[r3], cpu_env, cpu_gpr_d[r1]);
+        break;
     case OPC2_32_RR_FTOIZ:
         gen_helper_ftoiz(cpu_gpr_d[r3], cpu_env, cpu_gpr_d[r1]);
         break;
-- 
2.17.1

[Qemu-devel] [PATCH 3/5] tricore: fix RRPW_INSERT instruction

[David Brenken]

From: David Brenken <david.brenken@efs-auto.de>

Signed-off-by: Andreas Konopik <andreas.konopik@efs-auto.de>
Signed-off-by: David Brenken <david.brenken@efs-auto.de>
Signed-off-by: Georg Hofstetter <georg.hofstetter@efs-auto.de>
Signed-off-by: Robert Rasche <robert.rasche@efs-auto.de>
Signed-off-by: Lars Biermanski <lars.biermanski@efs-auto.de>

---
 target/tricore/translate.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index a8b4de647a..19d8db7a46 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -7004,6 +7004,7 @@ static void decode_rrpw_extract_insert(CPUTriCoreState *env, DisasContext *ctx)
     uint32_t op2;
     int r1, r2, r3;
     int32_t pos, width;
+    TCGv temp1, temp2;
 
     op2 = MASK_OP_RRPW_OP2(ctx->opcode);
     r1 = MASK_OP_RRPW_S1(ctx->opcode);
@@ -7042,9 +7043,13 @@ static void decode_rrpw_extract_insert(CPUTriCoreState *env, DisasContext *ctx)
         }
         break;
     case OPC2_32_RRPW_INSERT:
-        if (pos + width <= 31) {
-            tcg_gen_deposit_tl(cpu_gpr_d[r3], cpu_gpr_d[r1], cpu_gpr_d[r2],
-                               width, pos);
+        if (pos + width <= 32) {
+            temp1 = tcg_const_i32(pos);   /* pos */
+            temp2 = tcg_const_i32(width); /* width */
+            gen_insert(cpu_gpr_d[r3], cpu_gpr_d[r1], cpu_gpr_d[r2],
+                       temp1, temp2);
+            tcg_temp_free(temp1);
+            tcg_temp_free(temp2);
         }
         break;
     default:
-- 
2.17.1

[Qemu-devel] [PATCH 4/5] tricore: add QSEED instruction

[David Brenken]

From: Andreas Konopik <andreas.konopik@efs-auto.com>

Signed-off-by: Andreas Konopik <andreas.konopik@efs-auto.de>
Signed-off-by: David Brenken <david.brenken@efs-auto.de>
Signed-off-by: Georg Hofstetter <georg.hofstetter@efs-auto.de>
Signed-off-by: Robert Rasche <robert.rasche@efs-auto.de>
Signed-off-by: Lars Biermanski <lars.biermanski@efs-auto.de>

---
 target/tricore/fpu_helper.c | 88 +++++++++++++++++++++++++++++++++++++
 target/tricore/helper.h     |  1 +
 target/tricore/translate.c  |  3 ++
 3 files changed, 92 insertions(+)

diff --git a/target/tricore/fpu_helper.c b/target/tricore/fpu_helper.c
index 432079c8e2..68515ee3e0 100644
--- a/target/tricore/fpu_helper.c
+++ b/target/tricore/fpu_helper.c
@@ -24,6 +24,7 @@
 
 #define QUIET_NAN 0x7fc00000
 #define ADD_NAN   0x7fc00001
+#define SQRT_NAN  0x7fc00004
 #define DIV_NAN   0x7fc00008
 #define MUL_NAN   0x7fc00002
 #define FPU_FS PSW_USB_C
@@ -32,6 +33,9 @@
 #define FPU_FZ PSW_USB_AV
 #define FPU_FU PSW_USB_SAV
 
+#define float32_sqrt_nan make_float32(SQRT_NAN)
+#define float32_quiet_nan make_float32(QUIET_NAN)
+
 /* we don't care about input_denormal */
 static inline uint8_t f_get_excp_flags(CPUTriCoreState *env)
 {
@@ -166,6 +170,90 @@ uint32_t helper_fmul(CPUTriCoreState *env, uint32_t r1, uint32_t r2)
 
 }
 
+/*
+ * Target TriCore QSEED.F significand Lookup Table
+ *
+ * The QSEED.F output significand depends on the least-significant
+ * exponent bit and the 6 most-significant significand bits.
+ *
+ * IEEE 754 float datatype
+ * partitioned into Sign (S), Exponent (E) and Significand (M):
+ *
+ * S   E E E E E E E E   M M M M M M ...
+ *    |             |               |
+ *    +------+------+-------+-------+
+ *           |              |
+ *          for        lookup table
+ *      calculating     index for
+ *        output E       output M
+ */
+static const uint8_t target_qseed_significand_table[128] = {
+    253, 252, 245, 244, 239, 238, 231, 230, 225, 224, 217, 216,
+    211, 210, 205, 204, 201, 200, 195, 194, 189, 188, 185, 184,
+    179, 178, 175, 174, 169, 168, 165, 164, 161, 160, 157, 156,
+    153, 152, 149, 148, 145, 144, 141, 140, 137, 136, 133, 132,
+    131, 130, 127, 126, 123, 122, 121, 120, 117, 116, 115, 114,
+    111, 110, 109, 108, 103, 102, 99, 98, 93, 92, 89, 88, 83,
+    82, 79, 78, 75, 74, 71, 70, 67, 66, 63, 62, 59, 58, 55,
+    54, 53, 52, 49, 48, 45, 44, 43, 42, 39, 38, 37, 36, 33,
+    32, 31, 30, 27, 26, 25, 24, 23, 22, 19, 18, 17, 16, 15,
+    14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2
+};
+
+uint32_t helper_qseed(CPUTriCoreState *env, uint32_t r1)
+{
+    uint32_t flags;
+    uint32_t arg1, S, E, M, E_minus_one, m_idx;
+    uint32_t new_E, new_M, new_S, result;
+
+    arg1 = make_float32(r1);
+
+    /* fetch IEEE-754 fields S, E and the uppermost 6-bit of M */
+    S = extract32(arg1, 31, 1);
+    E = extract32(arg1, 23, 8);
+    M = extract32(arg1, 17, 6);
+
+    if (float32_is_any_nan(arg1)) {
+        result = float32_quiet_nan;
+        env->FPU_FI = 1;
+    } else if (E == 0) {
+        if (float32_is_neg(arg1)) {
+            result = float32_infinity | (1 << 31);
+        } else {
+            result = float32_infinity;
+        }
+    } else if (float32_is_neg(arg1)) {
+        result = float32_sqrt_nan;
+        env->FPU_FI = 1;
+    } else if (float32_is_infinity(arg1)) {
+        result = float32_zero;
+    } else {
+        E_minus_one = E - 1;
+        m_idx = ((E_minus_one & 1) << 6) | M;
+        new_S = S;
+        new_E = 0xBD - E_minus_one / 2;
+        new_M = target_qseed_significand_table[m_idx];
+
+        result = 0;
+        result = deposit32(result, 31, 1, new_S);
+        result = deposit32(result, 23, 8, new_E);
+        result = deposit32(result, 15, 8, new_M);
+    }
+
+    flags = f_get_excp_flags(env);
+    if (flags) {
+        if (flags & float_flag_invalid) {
+            f_update_psw_flags(env, flags);
+        } else {
+            env->FPU_FS = 0;
+        }
+    } else {
+        env->FPU_FS = 0;
+    }
+
+    return (uint32_t) result;
+}
+
 uint32_t helper_fdiv(CPUTriCoreState *env, uint32_t r1, uint32_t r2)
 {
     uint32_t flags;
diff --git a/target/tricore/helper.h b/target/tricore/helper.h
index f1a5cb367e..b64780c37d 100644
--- a/target/tricore/helper.h
+++ b/target/tricore/helper.h
@@ -109,6 +109,7 @@ DEF_HELPER_3(fdiv, i32, env, i32, i32)
 DEF_HELPER_4(fmadd, i32, env, i32, i32, i32)
 DEF_HELPER_4(fmsub, i32, env, i32, i32, i32)
 DEF_HELPER_3(fcmp, i32, env, i32, i32)
+DEF_HELPER_2(qseed, i32, env, i32)
 DEF_HELPER_2(ftoi, i32, env, i32)
 DEF_HELPER_2(itof, i32, env, i32)
 DEF_HELPER_2(utof, i32, env, i32)
diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index 19d8db7a46..dbc964901f 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -6764,6 +6764,9 @@ static void decode_rr_divide(CPUTriCoreState *env, DisasContext *ctx)
     case OPC2_32_RR_UPDFL:
         gen_helper_updfl(cpu_env, cpu_gpr_d[r1]);
         break;
+    case OPC2_32_RR_QSEED_F:
+        gen_helper_qseed(cpu_gpr_d[r3], cpu_env, cpu_gpr_d[r1]);
+        break;
     case OPC2_32_RR_UTOF:
         gen_helper_utof(cpu_gpr_d[r3], cpu_env, cpu_gpr_d[r1]);
         break;
-- 
2.17.1

[Qemu-devel] [PATCH 5/5] tricore: reset DisasContext before generating code

[David Brenken]

From: Georg Hofstetter <georg.hofstetter@efs-auto.de>

Signed-off-by: Andreas Konopik <andreas.konopik@efs-auto.de>
Signed-off-by: David Brenken <david.brenken@efs-auto.de>
Signed-off-by: Georg Hofstetter <georg.hofstetter@efs-auto.de>
Signed-off-by: Robert Rasche <robert.rasche@efs-auto.de>
Signed-off-by: Lars Biermanski <lars.biermanski@efs-auto.de>

---
 target/tricore/translate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index db09f82c31..cdbc00d654 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -8811,6 +8811,7 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
     target_ulong pc_start;
     int num_insns = 0;
 
+    memset(&ctx, 0x00, sizeof(DisasContext));
     pc_start = tb->pc;
     ctx.pc = pc_start;
     ctx.saved_pc = -1;
-- 
2.17.1

Re: [Qemu-devel] [PATCH 5/5] tricore: reset DisasContext before generating code

[Bastian Koppelmann]

Hi,

On 6/5/19 8:11 AM, David Brenken wrote:
> From: Georg Hofstetter <georg.hofstetter@efs-auto.de>
>
> Signed-off-by: Andreas Konopik <andreas.konopik@efs-auto.de>
> Signed-off-by: David Brenken <david.brenken@efs-auto.de>
> Signed-off-by: Georg Hofstetter <georg.hofstetter@efs-auto.de>
> Signed-off-by: Robert Rasche <robert.rasche@efs-auto.de>
> Signed-off-by: Lars Biermanski <lars.biermanski@efs-auto.de>
>
> ---
>   target/tricore/translate.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/target/tricore/translate.c b/target/tricore/translate.c
> index db09f82c31..cdbc00d654 100644
> --- a/target/tricore/translate.c
> +++ b/target/tricore/translate.c
> @@ -8811,6 +8811,7 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
>       target_ulong pc_start;
>       int num_insns = 0;
>   
> +    memset(&ctx, 0x00, sizeof(DisasContext));
>       pc_start = tb->pc;
>       ctx.pc = pc_start;
>       ctx.saved_pc = -1;

To me this looks like fixing a symptom instead of the root cause. Which 
commit did you bisect to? Do you have a reproducer?

Cheers,

Bastian

Re: [Qemu-devel] [PATCH 1/5] tricore: add FTOIZ instruction

[Bastian Koppelmann]

On 6/5/19 8:11 AM, David Brenken wrote:
> From: David Brenken <david.brenken@efs-auto.de>
>
> Signed-off-by: Andreas Konopik <andreas.konopik@efs-auto.de>
> Signed-off-by: David Brenken <david.brenken@efs-auto.de>
> Signed-off-by: Georg Hofstetter <georg.hofstetter@efs-auto.de>
> Signed-off-by: Robert Rasche <robert.rasche@efs-auto.de>
> Signed-off-by: Lars Biermanski <lars.biermanski@efs-auto.de>
>
> ---
>   target/tricore/fpu_helper.c | 25 +++++++++++++++++++++++++
>   target/tricore/helper.h     |  1 +
>   target/tricore/translate.c  |  3 +++
>   3 files changed, 29 insertions(+)

Reviewed-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>

Cheers,

Bastian

Re: [Qemu-devel] [PATCH 2/5] tricore: add UTOF instruction

[Bastian Koppelmann]

On 6/5/19 8:11 AM, David Brenken wrote:
> From: David Brenken <david.brenken@efs-auto.de>
>
> Signed-off-by: Andreas Konopik <andreas.konopik@efs-auto.de>
> Signed-off-by: David Brenken <david.brenken@efs-auto.de>
> Signed-off-by: Georg Hofstetter <georg.hofstetter@efs-auto.de>
> Signed-off-by: Robert Rasche <robert.rasche@efs-auto.de>
> Signed-off-by: Lars Biermanski <lars.biermanski@efs-auto.de>
>
> ---
>   target/tricore/fpu_helper.c | 16 ++++++++++++++++
>   target/tricore/helper.h     |  1 +
>   target/tricore/translate.c  |  3 +++
>   3 files changed, 20 insertions(+)
>
Reviewed-by: Bastian Koppelmann <kbastian@mail.uni-paderborn.de>

Cheers,
Bastian

Re: [Qemu-devel] [PATCH 3/5] tricore: fix RRPW_INSERT instruction

[Bastian Koppelmann]

Hi David,

On 6/5/19 8:11 AM, David Brenken wrote:
> From: David Brenken <david.brenken@efs-auto.de>
>
> Signed-off-by: Andreas Konopik <andreas.konopik@efs-auto.de>
> Signed-off-by: David Brenken <david.brenken@efs-auto.de>
> Signed-off-by: Georg Hofstetter <georg.hofstetter@efs-auto.de>
> Signed-off-by: Robert Rasche <robert.rasche@efs-auto.de>
> Signed-off-by: Lars Biermanski <lars.biermanski@efs-auto.de>
>
> ---
>   target/tricore/translate.c | 11 ++++++++---
>   1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/target/tricore/translate.c b/target/tricore/translate.c
> index a8b4de647a..19d8db7a46 100644
> --- a/target/tricore/translate.c
> +++ b/target/tricore/translate.c
> @@ -7004,6 +7004,7 @@ static void decode_rrpw_extract_insert(CPUTriCoreState *env, DisasContext *ctx)
>       uint32_t op2;
>       int r1, r2, r3;
>       int32_t pos, width;
> +    TCGv temp1, temp2;
>   
>       op2 = MASK_OP_RRPW_OP2(ctx->opcode);
>       r1 = MASK_OP_RRPW_S1(ctx->opcode);
> @@ -7042,9 +7043,13 @@ static void decode_rrpw_extract_insert(CPUTriCoreState *env, DisasContext *ctx)
>           }
>           break;
>       case OPC2_32_RRPW_INSERT:
> -        if (pos + width <= 31) {
> -            tcg_gen_deposit_tl(cpu_gpr_d[r3], cpu_gpr_d[r1], cpu_gpr_d[r2],
> -                               width, pos);
Can you explain the problem causing the bug? Deposit looks fine to me. 
After reading the specs again, I agree that the check needs to be <= 32.


> +        if (pos + width <= 32) {
> +            temp1 = tcg_const_i32(pos);   /* pos */
> +            temp2 = tcg_const_i32(width); /* width */
Useless comments.


Cheers,

Bastian

Re: [Qemu-devel] [PATCH 4/5] tricore: add QSEED instruction

[Bastian Koppelmann]

Hi,

On 6/5/19 8:11 AM, David Brenken wrote:
> +/*
> + * Target TriCore QSEED.F significand Lookup Table
> + *
> + * The QSEED.F output significand depends on the least-significant
> + * exponent bit and the 6 most-significant significand bits.
> + *
> + * IEEE 754 float datatype
> + * partitioned into Sign (S), Exponent (E) and Significand (M):
> + *
> + * S   E E E E E E E E   M M M M M M ...
> + *    |             |               |
> + *    +------+------+-------+-------+
> + *           |              |
> + *          for        lookup table
> + *      calculating     index for
> + *        output E       output M
> + */
> +static const uint8_t target_qseed_significand_table[128] = {
> +    253, 252, 245, 244, 239, 238, 231, 230, 225, 224, 217, 216,
> +    211, 210, 205, 204, 201, 200, 195, 194, 189, 188, 185, 184,
> +    179, 178, 175, 174, 169, 168, 165, 164, 161, 160, 157, 156,
> +    153, 152, 149, 148, 145, 144, 141, 140, 137, 136, 133, 132,
> +    131, 130, 127, 126, 123, 122, 121, 120, 117, 116, 115, 114,
> +    111, 110, 109, 108, 103, 102, 99, 98, 93, 92, 89, 88, 83,
> +    82, 79, 78, 75, 74, 71, 70, 67, 66, 63, 62, 59, 58, 55,
> +    54, 53, 52, 49, 48, 45, 44, 43, 42, 39, 38, 37, 36, 33,
> +    32, 31, 30, 27, 26, 25, 24, 23, 22, 19, 18, 17, 16, 15,
> +    14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2
> +};


Can you explain in a comment how you arrived at this lookup table?


> +    } else if (float32_is_neg(arg1)) {
> +        result = float32_sqrt_nan;
> +        env->FPU_FI = 1;
[...]
> +
> +    flags = f_get_excp_flags(env);
> +    if (flags) {
> +        if (flags & float_flag_invalid) {
> +            f_update_psw_flags(env, flags);
> +        } else {
> +            env->FPU_FS = 0;
> +        }
> +    } else {
> +        env->FPU_FS = 0;

You are setting FPU_FS to 0, even though FPU_FI might have been set in 
case of a NaN. I think it's best to remove the whole softfloat check, as 
none of the softfloat functions you call, can raise any flags.

Cheers,

Bastian

Re: [Qemu-devel] [PATCH 0/5] tricore: adding new instructions and fixing issues

[Bastian Koppelmann]

Hi Guys,

On 6/5/19 8:11 AM, David Brenken wrote:
> From: David Brenken <david.brenken@efs-auto.de>
>
> Hello everyone,
>
> in the scope of this patchset we added implementations for the following
> instructions for the target TriCore:
>
> QSEED, FTOIZ, UTOF

first of all thanks for these patches. QSEED.F was one instruction that 
was bugging me for some time and I didn't dare to do the grind figuring 
out how it works in all details.

I left some comments in the patch emails but in general I think the 
patches are nearly good to go. I created some scripts [1] to generate 
random tricore asm programs that I run against tsim. This has been very 
useful in catching bugs in the PSW calculation. You might want to run 
them as well :)


Cheers,
Bastian

[1] https://github.com/bkoppelmann/tricore-qemu-tests

Re: [Qemu-devel] [PATCH 3/5] tricore: fix RRPW_INSERT instruction

[Brenken, David (EFS-GH2)]

Hi Bastian,

> Hi David,
> 
> On 6/5/19 8:11 AM, David Brenken wrote:
> > From: David Brenken <david.brenken@efs-auto.de>
> >
> > Signed-off-by: Andreas Konopik <andreas.konopik@efs-auto.de>
> > Signed-off-by: David Brenken <david.brenken@efs-auto.de>
> > Signed-off-by: Georg Hofstetter <georg.hofstetter@efs-auto.de>
> > Signed-off-by: Robert Rasche <robert.rasche@efs-auto.de>
> > Signed-off-by: Lars Biermanski <lars.biermanski@efs-auto.de>
> >
> > ---
> >   target/tricore/translate.c | 11 ++++++++---
> >   1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/target/tricore/translate.c b/target/tricore/translate.c
> > index a8b4de647a..19d8db7a46 100644
> > --- a/target/tricore/translate.c
> > +++ b/target/tricore/translate.c
> > @@ -7004,6 +7004,7 @@ static void
> decode_rrpw_extract_insert(CPUTriCoreState *env, DisasContext *ctx)
> >       uint32_t op2;
> >       int r1, r2, r3;
> >       int32_t pos, width;
> > +    TCGv temp1, temp2;
> >
> >       op2 = MASK_OP_RRPW_OP2(ctx->opcode);
> >       r1 = MASK_OP_RRPW_S1(ctx->opcode);
> > @@ -7042,9 +7043,13 @@ static void
> decode_rrpw_extract_insert(CPUTriCoreState *env, DisasContext *ctx)
> >           }
> >           break;
> >       case OPC2_32_RRPW_INSERT:
> > -        if (pos + width <= 31) {
> > -            tcg_gen_deposit_tl(cpu_gpr_d[r3], cpu_gpr_d[r1], cpu_gpr_d[r2],
> > -                               width, pos);
> Can you explain the problem causing the bug? Deposit looks fine to me.
> After reading the specs again, I agree that the check needs to be <= 32.
The bug was recognized because of different behavior between actual hardware and QEMU.
Just from looking at it I would say that deposit masks and then shifts the arg2 (D[b]) while the 
manual states to first shift D[b] and then mask it. I remember that it was a corner case (e.g. 
width + pos = 31 or 32). 
I also thought that using the direct insert instruction is more convenient since it was present anyway.

> 
> 
> > +        if (pos + width <= 32) {
> > +            temp1 = tcg_const_i32(pos);   /* pos */
> > +            temp2 = tcg_const_i32(width); /* width */
> Useless comments.
I will remove them in a second version of the patchset.
> 
> 
> Cheers,
> 
> Bastian
Best regards

David

Re: [Qemu-devel] [PATCH 5/5] tricore: reset DisasContext before generating code

[Hofstetter, Georg (EFS-GH2)]

Hi Sebastian,

in translate.c:gen_mtcr() code accesses hflags within the structure:
    if ((ctx->hflags & TRICORE_HFLAG_KUU) == TRICORE_HFLAG_SM) {
        /* since we're caching PSW make this a special case */

The code expects the HFLAG set for kernel mode in (i guess) preparation for supporting operation modes.
There is no code modifying these flags.
The flags were introduced in 0aaeb11 and there it looks like it was expected to be zeroed - maybe you know a bit more.

As having a stack variable uninitialized is generally a bad idea, so my proposal was to zero it as a whole, as it would be for bss variables.
Alternatively we could initialize them explicitly to zero or TRICORE_HFLAG_SM.

    ctx.hflags = TRICORE_HFLAG_SM;

Not sure though why a cpu state flag is within DisasContext.

Regards,
Georg


-----Ursprüngliche Nachricht-----
Von: Bastian Koppelmann <kbastian@mail.uni-paderborn.de> 
Gesendet: Mittwoch, 5. Juni 2019 11:02
An: David Brenken <david.brenken@efs-auto.org>; qemu-devel@nongnu.org
Cc: Biermanski, Lars (EFS-GH3) <lars.biermanski@efs-auto.de>; Hofstetter, Georg (EFS-GH2) <Georg.Hofstetter@efs-auto.de>; Brenken, David (EFS-GH2) <david.brenken@efs-auto.de>; Rasche, Robert (EFS-GH2) <robert.rasche@efs-auto.de>; Konopik, Andreas (EFS-GH2) <andreas.konopik@efs-auto.de>
Betreff: Re: [Qemu-devel] [PATCH 5/5] tricore: reset DisasContext before generating code

Hi,

On 6/5/19 8:11 AM, David Brenken wrote:
> From: Georg Hofstetter <georg.hofstetter@efs-auto.de>
>
> Signed-off-by: Andreas Konopik <andreas.konopik@efs-auto.de>
> Signed-off-by: David Brenken <david.brenken@efs-auto.de>
> Signed-off-by: Georg Hofstetter <georg.hofstetter@efs-auto.de>
> Signed-off-by: Robert Rasche <robert.rasche@efs-auto.de>
> Signed-off-by: Lars Biermanski <lars.biermanski@efs-auto.de>
>
> ---
>   target/tricore/translate.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/target/tricore/translate.c b/target/tricore/translate.c 
> index db09f82c31..cdbc00d654 100644
> --- a/target/tricore/translate.c
> +++ b/target/tricore/translate.c
> @@ -8811,6 +8811,7 @@ void gen_intermediate_code(CPUState *cs, TranslationBlock *tb, int max_insns)
>       target_ulong pc_start;
>       int num_insns = 0;
>   
> +    memset(&ctx, 0x00, sizeof(DisasContext));
>       pc_start = tb->pc;
>       ctx.pc = pc_start;
>       ctx.saved_pc = -1;

To me this looks like fixing a symptom instead of the root cause. Which commit did you bisect to? Do you have a reproducer?

Cheers,

Bastian

Re: [Qemu-devel] [PATCH 5/5] tricore: reset DisasContext before generating code

[Bastian Koppelmann]

Hi Georg,

On 6/6/19 1:44 PM, Hofstetter, Georg (EFS-GH2) wrote:
> Hi Sebastian,
>
> in translate.c:gen_mtcr() code accesses hflags within the structure:
>      if ((ctx->hflags & TRICORE_HFLAG_KUU) == TRICORE_HFLAG_SM) {
>          /* since we're caching PSW make this a special case */
>
> The code expects the HFLAG set for kernel mode in (i guess) preparation for supporting operation modes.
> There is no code modifying these flags.
> The flags were introduced in 0aaeb11 and there it looks like it was expected to be zeroed - maybe you know a bit more.

Yep, the ctx->hflags is supposed to be synced by tb->flags (which is 
normally synced with CPUTriCoreState via cpu_get_tb_cpu_state()) in 
gen_intermediate_code(). Somehow I forgot to add the first sync. So, the 
proper fix is:

diff --git a/target/tricore/translate.c b/target/tricore/translate.c
index 06c4485e55..44296b3fb1 100644
--- a/target/tricore/translate.c
+++ b/target/tricore/translate.c
@@ -8804,6 +8804,7 @@ void gen_intermediate_code(CPUState *cs, 
TranslationBlock *tb, int max_insns)
      ctx.singlestep_enabled = cs->singlestep_enabled;
      ctx.bstate = BS_NONE;
      ctx.mem_idx = cpu_mmu_index(env, false);
+    ctx.hflags = (uint32_t)tb->flags;


      tcg_clear_temp_count();
      gen_tb_start(tb);


Cheers,

Bastian

Re: [Qemu-devel] [PATCH 4/5] tricore: add QSEED instruction

[Konopik, Andreas (EFS-GH2)]

Hi Bastian,

> Hi,
> 
> On 6/5/19 8:11 AM, David Brenken wrote:
> > +/*
> > + * Target TriCore QSEED.F significand Lookup Table
> > + *
> > + * The QSEED.F output significand depends on the least-significant
> > + * exponent bit and the 6 most-significant significand bits.
> > + *
> > + * IEEE 754 float datatype
> > + * partitioned into Sign (S), Exponent (E) and Significand (M):
> > + *
> > + * S   E E E E E E E E   M M M M M M ...
> > + *    |             |               |
> > + *    +------+------+-------+-------+
> > + *           |              |
> > + *          for        lookup table
> > + *      calculating     index for
> > + *        output E       output M
> > + */
> > +static const uint8_t target_qseed_significand_table[128] = {
> > +    253, 252, 245, 244, 239, 238, 231, 230, 225, 224, 217, 216,
> > +    211, 210, 205, 204, 201, 200, 195, 194, 189, 188, 185, 184,
> > +    179, 178, 175, 174, 169, 168, 165, 164, 161, 160, 157, 156,
> > +    153, 152, 149, 148, 145, 144, 141, 140, 137, 136, 133, 132,
> > +    131, 130, 127, 126, 123, 122, 121, 120, 117, 116, 115, 114,
> > +    111, 110, 109, 108, 103, 102, 99, 98, 93, 92, 89, 88, 83,
> > +    82, 79, 78, 75, 74, 71, 70, 67, 66, 63, 62, 59, 58, 55,
> > +    54, 53, 52, 49, 48, 45, 44, 43, 42, 39, 38, 37, 36, 33,
> > +    32, 31, 30, 27, 26, 25, 24, 23, 22, 19, 18, 17, 16, 15,
> > +    14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2 };
> 
> 
> Can you explain in a comment how you arrived at this lookup table?

We extracted this lookup table from real hardware results.
We also validated QEMU output values by comparing again with the real
hardware qseed output.
Testing was not made for all 2^32 possible input values,but for all
positive floats that yield different outputs (with a big stride).

We will add a shorter comment to the second version of the patchset

> > +    } else if (float32_is_neg(arg1)) {
> > +        result = float32_sqrt_nan;
> > +        env->FPU_FI = 1;
> [...]
> > +
> > +    flags = f_get_excp_flags(env);
> > +    if (flags) {
> > +        if (flags & float_flag_invalid) {
> > +            f_update_psw_flags(env, flags);
> > +        } else {
> > +            env->FPU_FS = 0;
> > +        }
> > +    } else {
> > +        env->FPU_FS = 0;
> 
> You are setting FPU_FS to 0, even though FPU_FI might have been set in case of
> a NaN. I think it's best to remove the whole softfloat check, as none of the
> softfloat functions you call, can raise any flags.

I will make the PSW update only dependent on is_NaN(input) and is_SQRT_NaN(output).

Best regards,

Andreas

Re: [Qemu-devel] [PATCH 3/5] tricore: fix RRPW_INSERT instruction

[Richard Henderson]

On 6/6/19 2:26 AM, Brenken, David (EFS-GH2) wrote:
>>>       case OPC2_32_RRPW_INSERT:
>>> -        if (pos + width <= 31) {
>>> -            tcg_gen_deposit_tl(cpu_gpr_d[r3], cpu_gpr_d[r1], cpu_gpr_d[r2],
>>> -                               width, pos);
>> Can you explain the problem causing the bug? Deposit looks fine to me.
>> After reading the specs again, I agree that the check needs to be <= 32.
> The bug was recognized because of different behavior between actual hardware and QEMU.
> Just from looking at it I would say that deposit masks and then shifts the arg2 (D[b]) while the 
> manual states to first shift D[b] and then mask it. I remember that it was a corner case (e.g. 
> width + pos = 31 or 32). 

The final two arguments to tcg_gen_deposit_tl are swapped.
It should be pos, width.


r~

Re: [Qemu-devel] [PATCH 3/5] tricore: fix RRPW_INSERT instruction

[Brenken, David (EFS-GH2)]

Thank you for your hint.
Would anyone mind, if keep my insert implementation anyway?
If I compare the pseudo code of the instruction manual to the insert implementation it looks nearly the same whereas it seems kind of difficult to directly map the pseudo code of the instruction manual to the general deposit instruction.

Best regards

David Brenken

Re: [Qemu-devel] [PATCH 3/5] tricore: fix RRPW_INSERT instruction

[Bastian Koppelmann]

Hi David,

On 6/12/19 7:48 AM, Brenken, David (EFS-GH2) wrote:
> Thank you for your hint.
> Would anyone mind, if keep my insert implementation anyway?
> If I compare the pseudo code of the instruction manual to the insert implementation it looks nearly the same whereas it seems kind of difficult to directly map the pseudo code of the instruction manual to the general deposit instruction.

I think deposit is better, as tcg can emit the native deposit 
instruction of the host processor (which it does for x86). gen_insert() 
on the other hand needs 8 tcg ops.

Regarding the pseudo code, take a look at tcg/README. You'll see that 
the semantics of insert and deposit are the same. Thus, I think Richards 
fix is better.

Cheers,

Bastian