diff mbox series

[v2,03/22] tcg/i386: Split out target constraints to tcg-target-con-str.h

Message ID 20210115210456.1053477-4-richard.henderson@linaro.org (mailing list archive)
State New, archived
Headers show
Series tcg: backend constraints cleanup | expand

Commit Message

Richard Henderson Jan. 15, 2021, 9:04 p.m. UTC
This eliminates the target-specific function target_parse_constraint
and folds it into the single caller, process_op_defs.  Since this is
done directly into the switch statement, duplicates are compilation
errors rather than silently ignored at runtime.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/i386/tcg-target-con-str.h |  33 +++++++++++
 tcg/i386/tcg-target.h         |   1 +
 tcg/tcg.c                     |  33 +++++++++--
 tcg/i386/tcg-target.c.inc     | 101 ++++++----------------------------
 4 files changed, 78 insertions(+), 90 deletions(-)
 create mode 100644 tcg/i386/tcg-target-con-str.h

Comments

Peter Maydell Jan. 19, 2021, 2:38 p.m. UTC | #1
On Fri, 15 Jan 2021 at 21:08, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> This eliminates the target-specific function target_parse_constraint
> and folds it into the single caller, process_op_defs.  Since this is
> done directly into the switch statement, duplicates are compilation
> errors rather than silently ignored at runtime.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  tcg/i386/tcg-target-con-str.h |  33 +++++++++++
>  tcg/i386/tcg-target.h         |   1 +
>  tcg/tcg.c                     |  33 +++++++++--
>  tcg/i386/tcg-target.c.inc     | 101 ++++++----------------------------
>  4 files changed, 78 insertions(+), 90 deletions(-)
>  create mode 100644 tcg/i386/tcg-target-con-str.h

> +REGS('r', ALL_GENERAL_REGS)
> +REGS('x', ALL_VECTOR_REGS)
> +REGS('q', ALL_BYTEL_REGS)     /* regs that can be used as a byte operand */
> +REGS('Q', ALL_BYTEH_REGS)     /* regs with a second byte (e.g. %ah) */
> +REGS('L', ALL_GENERAL_REGS & ~SOFTMMU_RESERVE_REGS)  /* qemu_ld/st */
> +REGS('s', ALL_BYTEL_REGS & ~SOFTMMU_RESERVE_REGS)    /* qemu_st8_i32 data */

In the new setup we define 's' as the BYTEL regs minus the
softmmu reserved ones...

> diff --git a/tcg/i386/tcg-target.c.inc b/tcg/i386/tcg-target.c.inc
> index 74637f654a..c4b0b6bfca 100644
> --- a/tcg/i386/tcg-target.c.inc
> +++ b/tcg/i386/tcg-target.c.inc
> @@ -132,6 +132,22 @@ static const int tcg_target_call_oarg_regs[] = {
>  # define TCG_REG_L1 TCG_REG_EDX
>  #endif
>
> +#define ALL_BYTEH_REGS         0x0000000fu
> +#if TCG_TARGET_REG_BITS == 64
> +# define ALL_GENERAL_REGS      0x0000ffffu
> +# define ALL_VECTOR_REGS       0xffff0000u
> +# define ALL_BYTEL_REGS        ALL_GENERAL_REGS
> +#else
> +# define ALL_GENERAL_REGS      0x000000ffu
> +# define ALL_VECTOR_REGS       0x00ff0000u
> +# define ALL_BYTEL_REGS        ALL_BYTEH_REGS
> +#endif
> +#ifdef CONFIG_SOFTMMU
> +# define SOFTMMU_RESERVE_REGS  ((1 << TCG_REG_L0) | (1 << TCG_REG_L1))
> +#else
> +# define SOFTMMU_RESERVE_REGS  0
> +#endif

...which (ignoring L0/L1) is going to be 0xffff on x86-64, and 0xf on i386.

> -    case 's':
> -        /* qemu_st8_i32 data constraint */
> -        ct->regs = 0xf;
> -#ifdef CONFIG_SOFTMMU
> -        tcg_regset_reset_reg(ct->regs, TCG_REG_L0);
> -        tcg_regset_reset_reg(ct->regs, TCG_REG_L1);
> -#endif
> -        break;

But in the old code the 's' constraint is 0xf for both
x86-64 and i386. To match that I think that the new constraint
should use BYTEH, not BYTEL:
REGS('s', ALL_BYTEH_REGS & ~SOFTMMU_RESERVE_REGS)

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM
Richard Henderson Jan. 19, 2021, 5:46 p.m. UTC | #2
On 1/19/21 4:38 AM, Peter Maydell wrote:
>> -    case 's':
>> -        /* qemu_st8_i32 data constraint */
>> -        ct->regs = 0xf;
>> -#ifdef CONFIG_SOFTMMU
>> -        tcg_regset_reset_reg(ct->regs, TCG_REG_L0);
>> -        tcg_regset_reset_reg(ct->regs, TCG_REG_L1);
>> -#endif
>> -        break;
> 
> But in the old code the 's' constraint is 0xf for both
> x86-64 and i386.

That's perhaps laziness, or simply the lack of names in the old code.  It
logically should be BYTEL, because that's where byte stores go from.

In the end it doesn't matter, because this constraint is *only* used by i386.
The opcode INDEX_op_qemu_st8_i32 is not used by x86_64 at all.  See
TCG_TARGET_HAS_qemu_st8_i32 in tcg-target.h.


r~
Peter Maydell Jan. 19, 2021, 6:07 p.m. UTC | #3
On Tue, 19 Jan 2021 at 17:46, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 1/19/21 4:38 AM, Peter Maydell wrote:
> >> -    case 's':
> >> -        /* qemu_st8_i32 data constraint */
> >> -        ct->regs = 0xf;
> >> -#ifdef CONFIG_SOFTMMU
> >> -        tcg_regset_reset_reg(ct->regs, TCG_REG_L0);
> >> -        tcg_regset_reset_reg(ct->regs, TCG_REG_L1);
> >> -#endif
> >> -        break;
> >
> > But in the old code the 's' constraint is 0xf for both
> > x86-64 and i386.
>
> That's perhaps laziness, or simply the lack of names in the old code.  It
> logically should be BYTEL, because that's where byte stores go from.
>
> In the end it doesn't matter, because this constraint is *only* used by i386.
> The opcode INDEX_op_qemu_st8_i32 is not used by x86_64 at all.  See
> TCG_TARGET_HAS_qemu_st8_i32 in tcg-target.h.

OK. Can we keep actual changes in separate commits from
just-refactoring changes, please?

thanks
-- PMM
diff mbox series

Patch

diff --git a/tcg/i386/tcg-target-con-str.h b/tcg/i386/tcg-target-con-str.h
new file mode 100644
index 0000000000..24e6bcb80d
--- /dev/null
+++ b/tcg/i386/tcg-target-con-str.h
@@ -0,0 +1,33 @@ 
+/* SPDX-License-Identifier: MIT */
+/*
+ * Define i386 target-specific operand constraints.
+ * Copyright (c) 2021 Linaro
+ *
+ */
+
+/*
+ * Define constraint letters for register sets:
+ * REGS(letter, register_mask)
+ */
+REGS('a', 1u << TCG_REG_EAX)
+REGS('b', 1u << TCG_REG_EBX)
+REGS('c', 1u << TCG_REG_ECX)
+REGS('d', 1u << TCG_REG_EDX)
+REGS('S', 1u << TCG_REG_ESI)
+REGS('D', 1u << TCG_REG_EDI)
+
+REGS('r', ALL_GENERAL_REGS)
+REGS('x', ALL_VECTOR_REGS)
+REGS('q', ALL_BYTEL_REGS)     /* regs that can be used as a byte operand */
+REGS('Q', ALL_BYTEH_REGS)     /* regs with a second byte (e.g. %ah) */
+REGS('L', ALL_GENERAL_REGS & ~SOFTMMU_RESERVE_REGS)  /* qemu_ld/st */
+REGS('s', ALL_BYTEL_REGS & ~SOFTMMU_RESERVE_REGS)    /* qemu_st8_i32 data */
+
+/*
+ * Define constraint letters for constants:
+ * CONST(letter, TCG_CT_CONST_* bit set)
+ */
+CONST('e', TCG_CT_CONST_S32)
+CONST('I', TCG_CT_CONST_I32)
+CONST('W', TCG_CT_CONST_WSZ)
+CONST('Z', TCG_CT_CONST_U32)
diff --git a/tcg/i386/tcg-target.h b/tcg/i386/tcg-target.h
index b693d3692d..77693e13ea 100644
--- a/tcg/i386/tcg-target.h
+++ b/tcg/i386/tcg-target.h
@@ -235,5 +235,6 @@  static inline void tb_target_set_jmp_target(uintptr_t tc_ptr, uintptr_t jmp_rx,
 #define TCG_TARGET_NEED_LDST_LABELS
 #endif
 #define TCG_TARGET_NEED_POOL_LABELS
+#define TCG_TARGET_CON_STR_H
 
 #endif
diff --git a/tcg/tcg.c b/tcg/tcg.c
index 8f8badb61c..2a85532589 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -103,8 +103,10 @@  static void tcg_register_jit_int(const void *buf, size_t size,
     __attribute__((unused));
 
 /* Forward declarations for functions declared and used in tcg-target.c.inc. */
+#ifndef TCG_TARGET_CON_STR_H
 static const char *target_parse_constraint(TCGArgConstraint *ct,
                                            const char *ct_str, TCGType type);
+#endif
 static void tcg_out_ld(TCGContext *s, TCGType type, TCGReg ret, TCGReg arg1,
                        intptr_t arg2);
 static bool tcg_out_mov(TCGContext *s, TCGType type, TCGReg ret, TCGReg arg);
@@ -2409,7 +2411,6 @@  static void process_op_defs(TCGContext *s)
     for (op = 0; op < NB_OPS; op++) {
         TCGOpDef *def = &tcg_op_defs[op];
         const TCGTargetOpDef *tdefs;
-        TCGType type;
         int i, nb_args;
 
         if (def->flags & TCG_OPF_NOT_PRESENT) {
@@ -2425,7 +2426,6 @@  static void process_op_defs(TCGContext *s)
         /* Missing TCGTargetOpDef entry. */
         tcg_debug_assert(tdefs != NULL);
 
-        type = (def->flags & TCG_OPF_64BIT ? TCG_TYPE_I64 : TCG_TYPE_I32);
         for (i = 0; i < nb_args; i++) {
             const char *ct_str = tdefs->args_ct_str[i];
             /* Incomplete TCGTargetOpDef entry. */
@@ -2457,11 +2457,34 @@  static void process_op_defs(TCGContext *s)
                     def->args_ct[i].ct |= TCG_CT_CONST;
                     ct_str++;
                     break;
+
+#ifdef TCG_TARGET_CON_STR_H
+                /* Include all of the target-specific constraints. */
+
+#undef CONST
+#define CONST(CASE, MASK) \
+    case CASE: def->args_ct[i].ct |= MASK; ct_str++; break;
+#define REGS(CASE, MASK) \
+    case CASE: def->args_ct[i].regs |= MASK; ct_str++; break;
+
+#include "tcg-target-con-str.h"
+
+#undef REGS
+#undef CONST
                 default:
-                    ct_str = target_parse_constraint(&def->args_ct[i],
-                                                     ct_str, type);
                     /* Typo in TCGTargetOpDef constraint. */
-                    tcg_debug_assert(ct_str != NULL);
+                    g_assert_not_reached();
+#else
+                default:
+                    {
+                        TCGType type = (def->flags & TCG_OPF_64BIT
+                                        ? TCG_TYPE_I64 : TCG_TYPE_I32);
+                        ct_str = target_parse_constraint(&def->args_ct[i],
+                                                         ct_str, type);
+                        /* Typo in TCGTargetOpDef constraint. */
+                        tcg_debug_assert(ct_str != NULL);
+                    }
+#endif
                 }
             }
         }
diff --git a/tcg/i386/tcg-target.c.inc b/tcg/i386/tcg-target.c.inc
index 74637f654a..c4b0b6bfca 100644
--- a/tcg/i386/tcg-target.c.inc
+++ b/tcg/i386/tcg-target.c.inc
@@ -132,6 +132,22 @@  static const int tcg_target_call_oarg_regs[] = {
 # define TCG_REG_L1 TCG_REG_EDX
 #endif
 
+#define ALL_BYTEH_REGS         0x0000000fu
+#if TCG_TARGET_REG_BITS == 64
+# define ALL_GENERAL_REGS      0x0000ffffu
+# define ALL_VECTOR_REGS       0xffff0000u
+# define ALL_BYTEL_REGS        ALL_GENERAL_REGS
+#else
+# define ALL_GENERAL_REGS      0x000000ffu
+# define ALL_VECTOR_REGS       0x00ff0000u
+# define ALL_BYTEL_REGS        ALL_BYTEH_REGS
+#endif
+#ifdef CONFIG_SOFTMMU
+# define SOFTMMU_RESERVE_REGS  ((1 << TCG_REG_L0) | (1 << TCG_REG_L1))
+#else
+# define SOFTMMU_RESERVE_REGS  0
+#endif
+
 /* The host compiler should supply <cpuid.h> to enable runtime features
    detection, as we're not going to go so far as our own inline assembly.
    If not available, default values will be assumed.  */
@@ -193,91 +209,6 @@  static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
     return true;
 }
 
-#if TCG_TARGET_REG_BITS == 64
-#define ALL_GENERAL_REGS   0x0000ffffu
-#define ALL_VECTOR_REGS    0xffff0000u
-#else
-#define ALL_GENERAL_REGS   0x000000ffu
-#define ALL_VECTOR_REGS    0x00ff0000u
-#endif
-
-/* parse target specific constraints */
-static const char *target_parse_constraint(TCGArgConstraint *ct,
-                                           const char *ct_str, TCGType type)
-{
-    switch(*ct_str++) {
-    case 'a':
-        tcg_regset_set_reg(ct->regs, TCG_REG_EAX);
-        break;
-    case 'b':
-        tcg_regset_set_reg(ct->regs, TCG_REG_EBX);
-        break;
-    case 'c':
-        tcg_regset_set_reg(ct->regs, TCG_REG_ECX);
-        break;
-    case 'd':
-        tcg_regset_set_reg(ct->regs, TCG_REG_EDX);
-        break;
-    case 'S':
-        tcg_regset_set_reg(ct->regs, TCG_REG_ESI);
-        break;
-    case 'D':
-        tcg_regset_set_reg(ct->regs, TCG_REG_EDI);
-        break;
-    case 'q':
-        /* A register that can be used as a byte operand.  */
-        ct->regs = TCG_TARGET_REG_BITS == 64 ? 0xffff : 0xf;
-        break;
-    case 'Q':
-        /* A register with an addressable second byte (e.g. %ah).  */
-        ct->regs = 0xf;
-        break;
-    case 'r':
-        /* A general register.  */
-        ct->regs |= ALL_GENERAL_REGS;
-        break;
-    case 'W':
-        /* With TZCNT/LZCNT, we can have operand-size as an input.  */
-        ct->ct |= TCG_CT_CONST_WSZ;
-        break;
-    case 'x':
-        /* A vector register.  */
-        ct->regs |= ALL_VECTOR_REGS;
-        break;
-
-    case 'L':
-        /* qemu_ld/st data+address constraint */
-        ct->regs = TCG_TARGET_REG_BITS == 64 ? 0xffff : 0xff;
-#ifdef CONFIG_SOFTMMU
-        tcg_regset_reset_reg(ct->regs, TCG_REG_L0);
-        tcg_regset_reset_reg(ct->regs, TCG_REG_L1);
-#endif
-        break;
-    case 's':
-        /* qemu_st8_i32 data constraint */
-        ct->regs = 0xf;
-#ifdef CONFIG_SOFTMMU
-        tcg_regset_reset_reg(ct->regs, TCG_REG_L0);
-        tcg_regset_reset_reg(ct->regs, TCG_REG_L1);
-#endif
-        break;
-
-    case 'e':
-        ct->ct |= TCG_CT_CONST_S32;
-        break;
-    case 'Z':
-        ct->ct |= TCG_CT_CONST_U32;
-        break;
-    case 'I':
-        ct->ct |= TCG_CT_CONST_I32;
-        break;
-
-    default:
-        return NULL;
-    }
-    return ct_str;
-}
-
 /* test if a constant matches the constraint */
 static inline int tcg_target_const_match(tcg_target_long val, TCGType type,
                                          const TCGArgConstraint *arg_ct)