[Qemu-devel] [PATCH 0/3] tcg/arm: fix division

[Qemu-devel] [PATCH 0/3] tcg/arm: fix division

[Aurelien Jarno]

The division on TCG ARM is currently broken due to to wrong constraints:
"1" and "2" are interpreted as arguments aliasing instead of 
constraints.

This could obviously be fixed, but the division on TCG ARM becomes quite
complex: for no obvious reason div2/divu2 are implemented instead of 
div/divu/rem/remu for no obvious reason and the code actually calls the
64-bit helpers for a 32-bit division.

This patch series fixes that by allowing TCG targets to use helpers for
the division in addition to the two current implementations.

[Qemu-devel] [PATCH 1/3] tcg: add div/rem 32-bit helpers

[Aurelien Jarno]

Some targets like ARM would benefit to use 32-bit helpers for
div/rem/divu/remu.

Create a #define for div2 so that targets can select between
div, div2 and helpers implementation. Use the helpers version if none
of the #define are present.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 tcg-runtime.c           |   24 +++++++++++++++++++
 tcg/arm/tcg-target.h    |    1 +
 tcg/hppa/tcg-target.h   |    1 +
 tcg/i386/tcg-target.h   |    1 +
 tcg/tcg-op.h            |   57 +++++++++++++++++++++++++++++++++++++++++++++-
 tcg/tcg-runtime.h       |    5 ++++
 tcg/x86_64/tcg-target.h |    2 +
 7 files changed, 89 insertions(+), 2 deletions(-)

diff --git a/tcg-runtime.c b/tcg-runtime.c
index 219cade..abfc364 100644
--- a/tcg-runtime.c
+++ b/tcg-runtime.c
@@ -25,6 +25,30 @@
 
 #include "tcg/tcg-runtime.h"
 
+/* 32-bit helpers */
+
+int32_t tcg_helper_div_i32(int32_t arg1, int32_t arg2)
+{
+    return arg1 / arg2;
+}
+
+int32_t tcg_helper_rem_i32(int32_t arg1, int32_t arg2)
+{
+    return arg1 % arg2;
+}
+
+uint32_t tcg_helper_divu_i32(uint32_t arg1, uint32_t arg2)
+{
+    return arg1 / arg2;
+}
+
+uint32_t tcg_helper_remu_i32(uint32_t arg1, uint32_t arg2)
+{
+    return arg1 % arg2;
+}
+
+/* 64-bit helpers */
+
 int64_t tcg_helper_shl_i64(int64_t arg1, int64_t arg2)
 {
     return arg1 << arg2;
diff --git a/tcg/arm/tcg-target.h b/tcg/arm/tcg-target.h
index 4cad967..4b2b0be 100644
--- a/tcg/arm/tcg-target.h
+++ b/tcg/arm/tcg-target.h
@@ -56,6 +56,7 @@ enum {
 #define TCG_TARGET_CALL_STACK_OFFSET	0
 
 /* optional instructions */
+#define TCG_TARGET_HAS_div2_i32
 #define TCG_TARGET_HAS_ext8s_i32
 #define TCG_TARGET_HAS_ext16s_i32
 // #define TCG_TARGET_HAS_ext8u_i32
diff --git a/tcg/hppa/tcg-target.h b/tcg/hppa/tcg-target.h
index 7ab6f0c..fa39bfc 100644
--- a/tcg/hppa/tcg-target.h
+++ b/tcg/hppa/tcg-target.h
@@ -75,6 +75,7 @@ enum {
 #define TCG_TARGET_STACK_GROWSUP
 
 /* optional instructions */
+#define TCG_TARGET_HAS_div2_i32
 //#define TCG_TARGET_HAS_ext8s_i32
 //#define TCG_TARGET_HAS_ext16s_i32
 //#define TCG_TARGET_HAS_bswap16_i32
diff --git a/tcg/i386/tcg-target.h b/tcg/i386/tcg-target.h
index f97034c..e994fd5 100644
--- a/tcg/i386/tcg-target.h
+++ b/tcg/i386/tcg-target.h
@@ -45,6 +45,7 @@ enum {
 #define TCG_TARGET_CALL_STACK_OFFSET 0
 
 /* optional instructions */
+#define TCG_TARGET_HAS_div2_i32
 #define TCG_TARGET_HAS_rot_i32
 #define TCG_TARGET_HAS_ext8s_i32
 #define TCG_TARGET_HAS_ext16s_i32
diff --git a/tcg/tcg-op.h b/tcg/tcg-op.h
index 6ae1760..c71e1a8 100644
--- a/tcg/tcg-op.h
+++ b/tcg/tcg-op.h
@@ -365,6 +365,19 @@ static inline void tcg_gen_helperN(void *func, int flags, int sizemask,
 }
 
 /* FIXME: Should this be pure?  */
+static inline void tcg_gen_helper32(void *func, TCGv_i32 ret,
+                                    TCGv_i32 a, TCGv_i32 b)
+{
+    TCGv_ptr fn;
+    TCGArg args[2];
+    fn = tcg_const_ptr((tcg_target_long)func);
+    args[0] = GET_TCGV_I32(a);
+    args[1] = GET_TCGV_I32(b);
+    tcg_gen_callN(&tcg_ctx, fn, 0, 0, GET_TCGV_I32(ret), 2, args);
+    tcg_temp_free_ptr(fn);
+}
+
+/* FIXME: Should this be pure?  */
 static inline void tcg_gen_helper64(void *func, TCGv_i64 ret,
                                     TCGv_i64 a, TCGv_i64 b)
 {
@@ -635,7 +648,7 @@ static inline void tcg_gen_remu_i32(TCGv_i32 ret, TCGv_i32 arg1, TCGv_i32 arg2)
 {
     tcg_gen_op3_i32(INDEX_op_remu_i32, ret, arg1, arg2);
 }
-#else
+#elif defined(TCG_TARGET_HAS_div2_i32)
 static inline void tcg_gen_div_i32(TCGv_i32 ret, TCGv_i32 arg1, TCGv_i32 arg2)
 {
     TCGv_i32 t0;
@@ -671,6 +684,26 @@ static inline void tcg_gen_remu_i32(TCGv_i32 ret, TCGv_i32 arg1, TCGv_i32 arg2)
     tcg_gen_op5_i32(INDEX_op_divu2_i32, t0, ret, arg1, t0, arg2);
     tcg_temp_free_i32(t0);
 }
+#else
+static inline void tcg_gen_div_i32(TCGv_i32 ret, TCGv_i32 arg1, TCGv_i32 arg2)
+{
+    tcg_gen_helper32(tcg_helper_div_i32, ret, arg1, arg2);
+}
+
+static inline void tcg_gen_rem_i32(TCGv_i32 ret, TCGv_i32 arg1, TCGv_i32 arg2)
+{
+    tcg_gen_helper32(tcg_helper_rem_i32, ret, arg1, arg2);
+}
+
+static inline void tcg_gen_divu_i32(TCGv_i32 ret, TCGv_i32 arg1, TCGv_i32 arg2)
+{
+    tcg_gen_helper32(tcg_helper_divu_i32, ret, arg1, arg2);
+}
+
+static inline void tcg_gen_remu_i32(TCGv_i32 ret, TCGv_i32 arg1, TCGv_i32 arg2)
+{
+    tcg_gen_helper32(tcg_helper_remu_i32, ret, arg1, arg2);
+}
 #endif
 
 #if TCG_TARGET_REG_BITS == 32
@@ -1135,7 +1168,7 @@ static inline void tcg_gen_remu_i64(TCGv_i64 ret, TCGv_i64 arg1, TCGv_i64 arg2)
 {
     tcg_gen_op3_i64(INDEX_op_remu_i64, ret, arg1, arg2);
 }
-#else
+#elif defined(TCG_TARGET_HAS_div2_i64)
 static inline void tcg_gen_div_i64(TCGv_i64 ret, TCGv_i64 arg1, TCGv_i64 arg2)
 {
     TCGv_i64 t0;
@@ -1171,6 +1204,26 @@ static inline void tcg_gen_remu_i64(TCGv_i64 ret, TCGv_i64 arg1, TCGv_i64 arg2)
     tcg_gen_op5_i64(INDEX_op_divu2_i64, t0, ret, arg1, t0, arg2);
     tcg_temp_free_i64(t0);
 }
+#else
+static inline void tcg_gen_div_i64(TCGv_i64 ret, TCGv_i64 arg1, TCGv_i64 arg2)
+{
+    tcg_gen_helper64(tcg_helper_div_i64, ret, arg1, arg2);
+}
+
+static inline void tcg_gen_rem_i64(TCGv_i64 ret, TCGv_i64 arg1, TCGv_i64 arg2)
+{
+    tcg_gen_helper64(tcg_helper_rem_i64, ret, arg1, arg2);
+}
+
+static inline void tcg_gen_divu_i64(TCGv_i64 ret, TCGv_i64 arg1, TCGv_i64 arg2)
+{
+    tcg_gen_helper64(tcg_helper_divu_i64, ret, arg1, arg2);
+}
+
+static inline void tcg_gen_remu_i64(TCGv_i64 ret, TCGv_i64 arg1, TCGv_i64 arg2)
+{
+    tcg_gen_helper64(tcg_helper_remu_i64, ret, arg1, arg2);
+}
 #endif
 
 #endif
diff --git a/tcg/tcg-runtime.h b/tcg/tcg-runtime.h
index e750cc1..5615b13 100644
--- a/tcg/tcg-runtime.h
+++ b/tcg/tcg-runtime.h
@@ -2,6 +2,11 @@
 #define TCG_RUNTIME_H
 
 /* tcg-runtime.c */
+int32_t tcg_helper_div_i32(int32_t arg1, int32_t arg2);
+int32_t tcg_helper_rem_i32(int32_t arg1, int32_t arg2);
+uint32_t tcg_helper_divu_i32(uint32_t arg1, uint32_t arg2);
+uint32_t tcg_helper_remu_i32(uint32_t arg1, uint32_t arg2);
+
 int64_t tcg_helper_shl_i64(int64_t arg1, int64_t arg2);
 int64_t tcg_helper_shr_i64(int64_t arg1, int64_t arg2);
 int64_t tcg_helper_sar_i64(int64_t arg1, int64_t arg2);
diff --git a/tcg/x86_64/tcg-target.h b/tcg/x86_64/tcg-target.h
index 765f0b4..d1e8b9e 100644
--- a/tcg/x86_64/tcg-target.h
+++ b/tcg/x86_64/tcg-target.h
@@ -56,6 +56,8 @@ enum {
 #define TCG_TARGET_CALL_STACK_OFFSET 0
 
 /* optional instructions */
+#define TCG_TARGET_HAS_div2_i32
+#define TCG_TARGET_HAS_div2_i64
 #define TCG_TARGET_HAS_bswap16_i32
 #define TCG_TARGET_HAS_bswap16_i64
 #define TCG_TARGET_HAS_bswap32_i32
-- 
1.7.0

[Qemu-devel] [PATCH 2/3] tcg/arm: use helpers for divu/remu

[Aurelien Jarno]

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 tcg/arm/tcg-target.c |   94 --------------------------------------------------
 tcg/arm/tcg-target.h |    1 -
 2 files changed, 0 insertions(+), 95 deletions(-)

diff --git a/tcg/arm/tcg-target.c b/tcg/arm/tcg-target.c
index 756f131..0ff8f99 100644
--- a/tcg/arm/tcg-target.c
+++ b/tcg/arm/tcg-target.c
@@ -157,19 +157,6 @@ static int target_parse_constraint(TCGArgConstraint *ct, const char **pct_str)
 # endif
 #endif
 
-    case '1':
-        ct->ct |= TCG_CT_REG;
-        tcg_regset_set32(ct->u.regs, 0, (1 << TCG_TARGET_NB_REGS) - 1);
-        tcg_regset_reset_reg(ct->u.regs, TCG_REG_R0);
-        break;
-
-    case '2':
-        ct->ct |= TCG_CT_REG;
-        tcg_regset_set32(ct->u.regs, 0, (1 << TCG_TARGET_NB_REGS) - 1);
-        tcg_regset_reset_reg(ct->u.regs, TCG_REG_R0);
-        tcg_regset_reset_reg(ct->u.regs, TCG_REG_R1);
-        break;
-
     default:
         return -1;
     }
@@ -819,75 +806,6 @@ static inline void tcg_out_goto_label(TCGContext *s, int cond, int label_index)
     }
 }
 
-static void tcg_out_div_helper(TCGContext *s, int cond, const TCGArg *args,
-                void *helper_div, void *helper_rem, int shift)
-{
-    int div_reg = args[0];
-    int rem_reg = args[1];
-
-    /* stmdb sp!, { r0 - r3, ip, lr } */
-    /* (Note that we need an even number of registers as per EABI) */
-    tcg_out32(s, (cond << 28) | 0x092d500f);
-
-    tcg_out_dat_reg(s, cond, ARITH_MOV, 0, 0, args[2], SHIFT_IMM_LSL(0));
-    tcg_out_dat_reg(s, cond, ARITH_MOV, 1, 0, args[3], SHIFT_IMM_LSL(0));
-    tcg_out_dat_reg(s, cond, ARITH_MOV, 2, 0, args[4], SHIFT_IMM_LSL(0));
-    tcg_out_dat_reg(s, cond, ARITH_MOV, 3, 0, 2, shift);
-
-    tcg_out_call(s, cond, (uint32_t) helper_div);
-    tcg_out_dat_reg(s, cond, ARITH_MOV, 8, 0, 0, SHIFT_IMM_LSL(0));
-
-    /* ldmia sp, { r0 - r3, fp, lr } */
-    tcg_out32(s, (cond << 28) | 0x089d500f);
-
-    tcg_out_dat_reg(s, cond, ARITH_MOV, 0, 0, args[2], SHIFT_IMM_LSL(0));
-    tcg_out_dat_reg(s, cond, ARITH_MOV, 1, 0, args[3], SHIFT_IMM_LSL(0));
-    tcg_out_dat_reg(s, cond, ARITH_MOV, 2, 0, args[4], SHIFT_IMM_LSL(0));
-    tcg_out_dat_reg(s, cond, ARITH_MOV, 3, 0, 2, shift);
-
-    tcg_out_call(s, cond, (uint32_t) helper_rem);
-
-    tcg_out_dat_reg(s, cond, ARITH_MOV, rem_reg, 0, 0, SHIFT_IMM_LSL(0));
-    tcg_out_dat_reg(s, cond, ARITH_MOV, div_reg, 0, 8, SHIFT_IMM_LSL(0));
-
-    /* ldr r0, [sp], #4 */
-    if (rem_reg != 0 && div_reg != 0) {
-        tcg_out32(s, (cond << 28) | 0x04bd0004);
-    } else {
-        tcg_out_dat_imm(s, cond, ARITH_ADD, 13, 13, 4);
-    }
-    /* ldr r1, [sp], #4 */
-    if (rem_reg != 1 && div_reg != 1) {
-        tcg_out32(s, (cond << 28) | 0x04bd1004);
-    } else {
-        tcg_out_dat_imm(s, cond, ARITH_ADD, 13, 13, 4);
-    }
-    /* ldr r2, [sp], #4 */
-    if (rem_reg != 2 && div_reg != 2) {
-        tcg_out32(s, (cond << 28) | 0x04bd2004);
-    } else {
-        tcg_out_dat_imm(s, cond, ARITH_ADD, 13, 13, 4);
-    }
-    /* ldr r3, [sp], #4 */
-    if (rem_reg != 3 && div_reg != 3) {
-        tcg_out32(s, (cond << 28) | 0x04bd3004);
-    } else {
-        tcg_out_dat_imm(s, cond, ARITH_ADD, 13, 13, 4);
-    }
-    /* ldr ip, [sp], #4 */
-    if (rem_reg != 12 && div_reg != 12) {
-        tcg_out32(s, (cond << 28) | 0x04bdc004);
-    } else {
-        tcg_out_dat_imm(s, cond, ARITH_ADD, 13, 13, 4);
-    }
-    /* ldr lr, [sp], #4 */
-    if (rem_reg != 14 && div_reg != 14) {
-        tcg_out32(s, (cond << 28) | 0x04bde004);
-    } else {
-        tcg_out_dat_imm(s, cond, ARITH_ADD, 13, 13, 4);
-    }
-}
-
 #ifdef CONFIG_SOFTMMU
 
 #include "../../softmmu_defs.h"
@@ -1487,16 +1405,6 @@ static inline void tcg_out_op(TCGContext *s, int opc,
     case INDEX_op_mulu2_i32:
         tcg_out_umull32(s, COND_AL, args[0], args[1], args[2], args[3]);
         break;
-    case INDEX_op_div2_i32:
-        tcg_out_div_helper(s, COND_AL, args,
-                        tcg_helper_div_i64, tcg_helper_rem_i64,
-                        SHIFT_IMM_ASR(31));
-        break;
-    case INDEX_op_divu2_i32:
-        tcg_out_div_helper(s, COND_AL, args,
-                        tcg_helper_divu_i64, tcg_helper_remu_i64,
-                        SHIFT_IMM_LSR(31));
-        break;
     /* XXX: Perhaps args[2] & 0x1f is wrong */
     case INDEX_op_shl_i32:
         c = const_args[2] ?
@@ -1652,8 +1560,6 @@ static const TCGTargetOpDef arm_op_defs[] = {
     { INDEX_op_sub_i32, { "r", "r", "rI" } },
     { INDEX_op_mul_i32, { "r", "r", "r" } },
     { INDEX_op_mulu2_i32, { "r", "r", "r", "r" } },
-    { INDEX_op_div2_i32, { "r", "r", "r", "1", "2" } },
-    { INDEX_op_divu2_i32, { "r", "r", "r", "1", "2" } },
     { INDEX_op_and_i32, { "r", "r", "rI" } },
     { INDEX_op_andc_i32, { "r", "r", "rI" } },
     { INDEX_op_or_i32, { "r", "r", "rI" } },
diff --git a/tcg/arm/tcg-target.h b/tcg/arm/tcg-target.h
index 4b2b0be..4cad967 100644
--- a/tcg/arm/tcg-target.h
+++ b/tcg/arm/tcg-target.h
@@ -56,7 +56,6 @@ enum {
 #define TCG_TARGET_CALL_STACK_OFFSET	0
 
 /* optional instructions */
-#define TCG_TARGET_HAS_div2_i32
 #define TCG_TARGET_HAS_ext8s_i32
 #define TCG_TARGET_HAS_ext16s_i32
 // #define TCG_TARGET_HAS_ext8u_i32
-- 
1.7.0

[Qemu-devel] [PATCH 3/3] tcg: declare internal helpers as const and pure

[Aurelien Jarno]

TCG internal helpers only access to the values passed in arguments, and
do not modify the CPU internal state. Thus they can be declared as
const and pure.

Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
---
 tcg/tcg-op.h |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tcg/tcg-op.h b/tcg/tcg-op.h
index c71e1a8..30a7a73 100644
--- a/tcg/tcg-op.h
+++ b/tcg/tcg-op.h
@@ -364,7 +364,6 @@ static inline void tcg_gen_helperN(void *func, int flags, int sizemask,
     tcg_temp_free_ptr(fn);
 }
 
-/* FIXME: Should this be pure?  */
 static inline void tcg_gen_helper32(void *func, TCGv_i32 ret,
                                     TCGv_i32 a, TCGv_i32 b)
 {
@@ -373,11 +372,11 @@ static inline void tcg_gen_helper32(void *func, TCGv_i32 ret,
     fn = tcg_const_ptr((tcg_target_long)func);
     args[0] = GET_TCGV_I32(a);
     args[1] = GET_TCGV_I32(b);
-    tcg_gen_callN(&tcg_ctx, fn, 0, 0, GET_TCGV_I32(ret), 2, args);
+    tcg_gen_callN(&tcg_ctx, fn, TCG_CALL_CONST | TCG_CALL_PURE,
+                  0, GET_TCGV_I32(ret), 2, args);
     tcg_temp_free_ptr(fn);
 }
 
-/* FIXME: Should this be pure?  */
 static inline void tcg_gen_helper64(void *func, TCGv_i64 ret,
                                     TCGv_i64 a, TCGv_i64 b)
 {
@@ -386,7 +385,8 @@ static inline void tcg_gen_helper64(void *func, TCGv_i64 ret,
     fn = tcg_const_ptr((tcg_target_long)func);
     args[0] = GET_TCGV_I64(a);
     args[1] = GET_TCGV_I64(b);
-    tcg_gen_callN(&tcg_ctx, fn, 0, 7, GET_TCGV_I64(ret), 2, args);
+    tcg_gen_callN(&tcg_ctx, fn, TCG_CALL_CONST | TCG_CALL_PURE,
+                  7, GET_TCGV_I64(ret), 2, args);
     tcg_temp_free_ptr(fn);
 }
 
-- 
1.7.0

Re: [Qemu-devel] [PATCH 3/3] tcg: declare internal helpers as const and pure

[Paul Brook]

> TCG internal helpers only access to the values passed in arguments, and
> do not modify the CPU internal state. Thus they can be declared as
> const and pure.

I think this needs an explanatory comment. It's not immediately obvious that 
tcg_gen_helperN and tcg_gen_helper{32,64} have significantly different 
semantics.

tcg/README also needs updating, specifically:

"* Helpers:

Using the tcg_gen_helper_x_y it is possible to call any function
taking i32, i64 or pointer types. Before calling an helper, all
globals are stored at their canonical location and it is assumed that
the function can modify them. In the future, function modifiers will
be allowed to tell that the helper does not read or write some globals.
"

Paul

Re: [Qemu-devel] [PATCH 3/3] tcg: declare internal helpers as const and pure

[Aurelien Jarno]

On Fri, Mar 05, 2010 at 11:15:45AM +0000, Paul Brook wrote:
> > TCG internal helpers only access to the values passed in arguments, and
> > do not modify the CPU internal state. Thus they can be declared as
> > const and pure.
> 
> I think this needs an explanatory comment. It's not immediately obvious that 
> tcg_gen_helperN and tcg_gen_helper{32,64} have significantly different 
> semantics.

What do you mean exactly? Mentioning explicitly tcg_gen_helper{32,64}
instead of "TCG internal helpers".

> tcg/README also needs updating, specifically:
> 
> "* Helpers:
> 
> Using the tcg_gen_helper_x_y it is possible to call any function
> taking i32, i64 or pointer types. Before calling an helper, all
> globals are stored at their canonical location and it is assumed that
> the function can modify them. In the future, function modifiers will
> be allowed to tell that the helper does not read or write some globals.
> "
> 

Fully agreed, but it is not a change introduce by this patch series. 
I'll submit a patch later.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 3/3] tcg: declare internal helpers as const and pure

[Paul Brook]

> On Fri, Mar 05, 2010 at 11:15:45AM +0000, Paul Brook wrote:
> > > TCG internal helpers only access to the values passed in arguments, and
> > > do not modify the CPU internal state. Thus they can be declared as
> > > const and pure.
> >
> > I think this needs an explanatory comment. It's not immediately obvious
> > that tcg_gen_helperN and tcg_gen_helper{32,64} have significantly
> > different semantics.
> 
> What do you mean exactly? Mentioning explicitly tcg_gen_helper{32,64}
> instead of "TCG internal helpers".

I think the difference between tcg_gen_helperN and tcg_gen_helper{32,64} is 
sufficiently subtle that it deserves documenting.  It's not obvious that the 
latter may only be used for cont/pure helpers.  My guess is that the FIXME 
you're removing was added precisely because there was uncertainty whether this 
assumption was reasonable, and under which circumstances they are used.

Paul