[Qemu-devel] [PATCH 0/2] target-arm: inline abs, 64-bit negate

[Qemu-devel] [PATCH 0/2] target-arm: inline abs, 64-bit negate

[Peter Maydell]

Two minor patches which inline some operations rather than using helper
functions. The 64 bit negate is a no-brainer since there's a simple
TCG op for it. For abs we implement in terms of movcond:
 movi_i32 tmp6,$0x0
 neg_i32 tmp7,tmp5
 movcond_i32 tmp5,tmp5,tmp6,tmp5,tmp7,gt
which the x86-64 backend turns into:
 0x603b53a7:  mov    %ebp,%ebx
 0x603b53a9:  neg    %ebx
 0x603b53ab:  mov    %ebp,%r12d
 0x603b53ae:  test   %ebp,%ebp
 0x603b53b0:  cmovle %ebx,%r12d

Not sure why it felt the need to use an extra move there, but
for ARM this isn't in a particularly performance critical bit
of the instruction set (it's a Neon operation) so I'm not too
worried.

(A fully native TCG abs op would be able to use the fact that
neg sets flags to avoid the test as well.)

Peter Maydell (2):
  target-arm: Use TCG operation for Neon 64 bit negation
  target-arm: Implement abs_i32 inline rather than as a helper

 target-arm/helper.c      |    5 -----
 target-arm/helper.h      |    2 --
 target-arm/neon_helper.c |    6 ------
 target-arm/translate.c   |   14 +++++++++++---
 4 files changed, 11 insertions(+), 16 deletions(-)

-- 
1.7.9.5

[Qemu-devel] [PATCH 1/2] target-arm: Use TCG operation for Neon 64 bit negation

[Peter Maydell]

Use the TCG operation to do Neon 64 bit negations rather than calling
a helper routine for it.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.h      |    1 -
 target-arm/neon_helper.c |    6 ------
 target-arm/translate.c   |    4 +++-
 3 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/target-arm/helper.h b/target-arm/helper.h
index 8b9adf1..fa3472f 100644
--- a/target-arm/helper.h
+++ b/target-arm/helper.h
@@ -339,7 +339,6 @@ DEF_HELPER_2(neon_mull_s16, i64, i32, i32)
 
 DEF_HELPER_1(neon_negl_u16, i64, i64)
 DEF_HELPER_1(neon_negl_u32, i64, i64)
-DEF_HELPER_1(neon_negl_u64, i64, i64)
 
 DEF_HELPER_2(neon_qabs_s8, i32, env, i32)
 DEF_HELPER_2(neon_qabs_s16, i32, env, i32)
diff --git a/target-arm/neon_helper.c b/target-arm/neon_helper.c
index 8bb5129..616cf95 100644
--- a/target-arm/neon_helper.c
+++ b/target-arm/neon_helper.c
@@ -1665,12 +1665,6 @@ uint64_t HELPER(neon_negl_u32)(uint64_t x)
     return low | ((uint64_t)high << 32);
 }
 
-/* FIXME:  There should be a native op for this.  */
-uint64_t HELPER(neon_negl_u64)(uint64_t x)
-{
-    return -x;
-}
-
 /* Saturating sign manipulation.  */
 /* ??? Make these use NEON_VOP1 */
 #define DO_QABS8(x) do { \
diff --git a/target-arm/translate.c b/target-arm/translate.c
index daccb15..d33f94c 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -4184,7 +4184,9 @@ static inline void gen_neon_negl(TCGv_i64 var, int size)
     switch (size) {
     case 0: gen_helper_neon_negl_u16(var, var); break;
     case 1: gen_helper_neon_negl_u32(var, var); break;
-    case 2: gen_helper_neon_negl_u64(var, var); break;
+    case 2:
+        tcg_gen_neg_i64(var, var);
+        break;
     default: abort();
     }
 }
-- 
1.7.9.5

[Qemu-devel] [PATCH 2/2] target-arm: Implement abs_i32 inline rather than as a helper

[Peter Maydell]

Implement abs_i32 inline (with movcond) rather than using a helper
function.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c    |    5 -----
 target-arm/helper.h    |    1 -
 target-arm/translate.c |   10 ++++++++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 58340bd..0601760 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1562,11 +1562,6 @@ uint32_t HELPER(rbit)(uint32_t x)
     return x;
 }
 
-uint32_t HELPER(abs)(uint32_t x)
-{
-    return ((int32_t)x < 0) ? -x : x;
-}
-
 #if defined(CONFIG_USER_ONLY)
 
 void do_interrupt (CPUARMState *env)
diff --git a/target-arm/helper.h b/target-arm/helper.h
index fa3472f..60812de 100644
--- a/target-arm/helper.h
+++ b/target-arm/helper.h
@@ -13,7 +13,6 @@ DEF_HELPER_2(double_saturate, i32, env, s32)
 DEF_HELPER_FLAGS_2(sdiv, TCG_CALL_CONST | TCG_CALL_PURE, s32, s32, s32)
 DEF_HELPER_FLAGS_2(udiv, TCG_CALL_CONST | TCG_CALL_PURE, i32, i32, i32)
 DEF_HELPER_FLAGS_1(rbit, TCG_CALL_CONST | TCG_CALL_PURE, i32, i32)
-DEF_HELPER_FLAGS_1(abs, TCG_CALL_CONST | TCG_CALL_PURE, i32, i32)
 
 #define PAS_OP(pfx)  \
     DEF_HELPER_3(pfx ## add8, i32, i32, i32, ptr) \
diff --git a/target-arm/translate.c b/target-arm/translate.c
index d33f94c..5a7602a 100644
--- a/target-arm/translate.c
+++ b/target-arm/translate.c
@@ -462,8 +462,14 @@ static void gen_sar(TCGv dest, TCGv t0, TCGv t1)
     tcg_temp_free_i32(tmp1);
 }
 
-/* FIXME:  Implement this natively.  */
-#define tcg_gen_abs_i32(t0, t1) gen_helper_abs(t0, t1)
+static void tcg_gen_abs_i32(TCGv dest, TCGv src)
+{
+    TCGv c0 = tcg_const_i32(0);
+    printf("tcg_gen_abs_i32\n");
+    tcg_gen_neg_i32(dest, src);
+    tcg_gen_movcond_i32(TCG_COND_GT, dest, src, c0, src, dest);
+    tcg_temp_free_i32(c0);
+}
 
 static void shifter_out_im(TCGv var, int shift)
 {
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH 2/2] target-arm: Implement abs_i32 inline rather than as a helper

[malc]

On Thu, 18 Oct 2012, Peter Maydell wrote:

[..snip..]

>  
> -/* FIXME:  Implement this natively.  */
> -#define tcg_gen_abs_i32(t0, t1) gen_helper_abs(t0, t1)
> +static void tcg_gen_abs_i32(TCGv dest, TCGv src)
> +{
> +    TCGv c0 = tcg_const_i32(0);
> +    printf("tcg_gen_abs_i32\n");

       ^^^^^^  doubt it was intentional

> +    tcg_gen_neg_i32(dest, src);
> +    tcg_gen_movcond_i32(TCG_COND_GT, dest, src, c0, src, dest);
> +    tcg_temp_free_i32(c0);
> +}
>  
>  static void shifter_out_im(TCGv var, int shift)
>  {
> 

-- 
mailto:av1474@comtv.ru

Re: [Qemu-devel] [PATCH 2/2] target-arm: Implement abs_i32 inline rather than as a helper

[Peter Maydell]

On 18 October 2012 18:40, malc <av1474@comtv.ru> wrote:
> On Thu, 18 Oct 2012, Peter Maydell wrote:
>
> [..snip..]
>
>>
>> -/* FIXME:  Implement this natively.  */
>> -#define tcg_gen_abs_i32(t0, t1) gen_helper_abs(t0, t1)
>> +static void tcg_gen_abs_i32(TCGv dest, TCGv src)
>> +{
>> +    TCGv c0 = tcg_const_i32(0);
>> +    printf("tcg_gen_abs_i32\n");
>
>        ^^^^^^  doubt it was intentional

Indeed, thanks. I seem to be making a lot of silly errors today...

-- PMM