Re: [PATCH v2 03/22] tcg/i386: Split out target constraints to tcg-target-con-str.h
On Tue, 19 Jan 2021 at 17:46, Richard Henderson 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
Re: [PATCH v2 03/22] tcg/i386: Split out target constraints to tcg-target-con-str.h
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~
Re: [PATCH v2 03/22] tcg/i386: Split out target constraints to tcg-target-con-str.h
On Fri, 15 Jan 2021 at 21:08, Richard Henderson
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
> ---
> 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 0x000fu
> +#if TCG_TARGET_REG_BITS == 64
> +# define ALL_GENERAL_REGS 0xu
> +# define ALL_VECTOR_REGS 0xu
> +# define ALL_BYTEL_REGSALL_GENERAL_REGS
> +#else
> +# define ALL_GENERAL_REGS 0x00ffu
> +# define ALL_VECTOR_REGS 0x00ffu
> +# define ALL_BYTEL_REGSALL_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 0x 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
thanks
-- PMM
[PATCH v2 03/22] tcg/i386: Split out target constraints to tcg-target-con-str.h
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
---
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
diff --git a/tcg/i386/tcg-target-con-str.h b/tcg/i386/tcg-target-con-str.h
new file mode 100644
index 00..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 = _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(>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(>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
