Re: [Qemu-devel] [PATCH 15/67] target/arm: Convert Saturating addition and subtraction

2019-08-05 Thread Peter Maydell 
On Fri, 26 Jul 2019 at 18:50, Richard Henderson
 wrote:
>
> Signed-off-by: Richard Henderson 
> ---
>  target/arm/helper.h|  1 -
>  target/arm/op_helper.c | 15 -
>  target/arm/translate.c | 74 +++---
>  target/arm/a32.decode  | 10 ++
>  target/arm/t32.decode  |  9 +
>  5 files changed, 66 insertions(+), 43 deletions(-)
>
> +/*
> + * Saturating addition and subtraction
> + */
> +
> +static bool op_qaddsub(DisasContext *s, arg_rrr *a, bool add, bool doub)
> +{
> +TCGv_i32 t0, t1;
> +
> +if (s->thumb
> +? !arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)
> +: !ENABLE_ARCH_5TE) {
> +return false;
> +}
> +
> +t0 = load_reg(s, a->rm);
> +t1 = load_reg(s, a->rn);
> +if (doub) {
> +gen_helper_add_saturate(t1, cpu_env, t1, t1);
> +}
> +if (add) {
> +gen_helper_add_saturate(t0, cpu_env, t0, t1);
> +} else {
> +gen_helper_sub_saturate(t0, cpu_env, t0, t1);
> +}
> +tcg_temp_free_i32(t1);
> +store_reg(s, a->rd, t0);
> +return true;
> +}
> +

> -case 0x5: /* saturating add/subtract */
> -ARCH(5TE);
> -rd = (insn >> 12) & 0xf;
> -rn = (insn >> 16) & 0xf;
> -tmp = load_reg(s, rm);
> -tmp2 = load_reg(s, rn);
> -if (op1 & 2)
> -gen_helper_double_saturate(tmp2, cpu_env, tmp2);
> -if (op1 & 1)
> -gen_helper_sub_saturate(tmp, cpu_env, tmp, tmp2);
> -else
> -gen_helper_add_saturate(tmp, cpu_env, tmp, tmp2);
> -tcg_temp_free_i32(tmp2);
> -store_reg(s, rd, tmp);
> -break;

This is changing the way we generate code in the middle
of also doing the refactoring. Could you not do this,
please (or where it really does make sense to do it then
call it out in the commit message)? It makes it harder
to review because now I have to read the patch for two
different changes at once...

thanks
-- PMM

[Qemu-devel] [PATCH 15/67] target/arm: Convert Saturating addition and subtraction

2019-07-26 Thread Richard Henderson 
Signed-off-by: Richard Henderson 
---
 target/arm/helper.h|  1 -
 target/arm/op_helper.c | 15 -
 target/arm/translate.c | 74 +++---
 target/arm/a32.decode  | 10 ++
 target/arm/t32.decode  |  9 +
 5 files changed, 66 insertions(+), 43 deletions(-)

diff --git a/target/arm/helper.h b/target/arm/helper.h
index 132aa1682e..1fb2cb5a77 100644
--- a/target/arm/helper.h
+++ b/target/arm/helper.h
@@ -6,7 +6,6 @@ DEF_HELPER_3(add_saturate, i32, env, i32, i32)
 DEF_HELPER_3(sub_saturate, i32, env, i32, i32)
 DEF_HELPER_3(add_usaturate, i32, env, i32, i32)
 DEF_HELPER_3(sub_usaturate, i32, env, i32, i32)
-DEF_HELPER_2(double_saturate, i32, env, s32)
 DEF_HELPER_FLAGS_2(sdiv, TCG_CALL_NO_RWG_SE, s32, s32, s32)
 DEF_HELPER_FLAGS_2(udiv, TCG_CALL_NO_RWG_SE, i32, i32, i32)
 DEF_HELPER_FLAGS_1(rbit, TCG_CALL_NO_RWG_SE, i32, i32)
diff --git a/target/arm/op_helper.c b/target/arm/op_helper.c
index 1ab91f915e..142239b03a 100644
--- a/target/arm/op_helper.c
+++ b/target/arm/op_helper.c
@@ -135,21 +135,6 @@ uint32_t HELPER(sub_saturate)(CPUARMState *env, uint32_t 
a, uint32_t b)
 return res;
 }
 
-uint32_t HELPER(double_saturate)(CPUARMState *env, int32_t val)
-{
-uint32_t res;
-if (val >= 0x4000) {
-res = ~SIGNBIT;
-env->QF = 1;
-} else if (val <= (int32_t)0xc000) {
-res = SIGNBIT;
-env->QF = 1;
-} else {
-res = val << 1;
-}
-return res;
-}
-
 uint32_t HELPER(add_usaturate)(CPUARMState *env, uint32_t a, uint32_t b)
 {
 uint32_t res = a + b;
diff --git a/target/arm/translate.c b/target/arm/translate.c
index 354a52d36c..85f829c1bb 100644
--- a/target/arm/translate.c
+++ b/target/arm/translate.c
@@ -8174,6 +8174,47 @@ static bool trans_UMAAL(DisasContext *s, arg_UMAAL *a)
 return true;
 }
 
+/*
+ * Saturating addition and subtraction
+ */
+
+static bool op_qaddsub(DisasContext *s, arg_rrr *a, bool add, bool doub)
+{
+TCGv_i32 t0, t1;
+
+if (s->thumb
+? !arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)
+: !ENABLE_ARCH_5TE) {
+return false;
+}
+
+t0 = load_reg(s, a->rm);
+t1 = load_reg(s, a->rn);
+if (doub) {
+gen_helper_add_saturate(t1, cpu_env, t1, t1);
+}
+if (add) {
+gen_helper_add_saturate(t0, cpu_env, t0, t1);
+} else {
+gen_helper_sub_saturate(t0, cpu_env, t0, t1);
+}
+tcg_temp_free_i32(t1);
+store_reg(s, a->rd, t0);
+return true;
+}
+
+#define DO_QADDSUB(NAME, ADD, DOUB) \
+static bool trans_##NAME(DisasContext *s, arg_rrr *a)\
+{\
+return op_qaddsub(s, a, ADD, DOUB);  \
+}
+
+DO_QADDSUB(QADD, true, false)
+DO_QADDSUB(QSUB, false, false)
+DO_QADDSUB(QDADD, true, true)
+DO_QADDSUB(QDSUB, false, true)
+
+#undef DO_QADDSUB
 
 /*
  * Legacy decoder.
@@ -8582,21 +8623,10 @@ static void disas_arm_insn(DisasContext *s, unsigned 
int insn)
 store_reg(s, rd, tmp);
 break;
 }
-case 0x5: /* saturating add/subtract */
-ARCH(5TE);
-rd = (insn >> 12) & 0xf;
-rn = (insn >> 16) & 0xf;
-tmp = load_reg(s, rm);
-tmp2 = load_reg(s, rn);
-if (op1 & 2)
-gen_helper_double_saturate(tmp2, cpu_env, tmp2);
-if (op1 & 1)
-gen_helper_sub_saturate(tmp, cpu_env, tmp, tmp2);
-else
-gen_helper_add_saturate(tmp, cpu_env, tmp, tmp2);
-tcg_temp_free_i32(tmp2);
-store_reg(s, rd, tmp);
-break;
+case 0x5:
+/* Saturating addition and subtraction.  */
+/* All done in decodetree.  Reach here for illegal ops.  */
+goto illegal_op;
 case 0x6: /* ERET */
 if (op1 != 3) {
 goto illegal_op;
@@ -10070,18 +10100,8 @@ static void disas_thumb2_insn(DisasContext *s, 
uint32_t insn)
 op = ((insn >> 17) & 0x38) | ((insn >> 4) & 7);
 if (op < 4) {
 /* Saturating add/subtract.  */
-if (!arm_dc_feature(s, ARM_FEATURE_THUMB_DSP)) {
-goto illegal_op;
-}
-tmp = load_reg(s, rn);
-tmp2 = load_reg(s, rm);
-if (op & 1)
-gen_helper_double_saturate(tmp, cpu_env, tmp);
-if (op & 2)
-gen_helper_sub_saturate(tmp, cpu_env, tmp2, tmp);
-else
-gen_helper_add_saturate(tmp, cpu_env, tmp, tmp2);
-tcg_temp_free_i32(tmp2);
+/* All done in decodetree.  Reach here for illegal ops.  */
+goto illegal_op;
 } else {
 switch (op) {
 case 0x0a: /* rbit */
diff --git a/target/arm/a32.decode b/target/arm/a32.decode
index 71846b79fd..af6712a9e8 100644
--- a/target/arm/a32.decode
+++