diff mbox series

[v6,04/16] tcg/ppc: Enable tcg backend vector compilation

Message ID 20190629130017.2973-5-richard.henderson@linaro.org
State Superseded
Headers show
Series tcg/ppc: Add vector opcodes | expand

Commit Message

Richard Henderson June 29, 2019, 1 p.m. UTC
Introduce all of the flags required to enable tcg backend vector support,
and a runtime flag to indicate the host supports Altivec instructions.

For now, do not actually set have_isa_altivec to true, because we have not
yet added all of the code to actually generate all of the required insns.
However, we must define these flags in order to disable ifndefs that create
stub versions of the functions added here.

The change to tcg_out_movi works around a buglet in tcg.c wherein if we
do not define tcg_out_dupi_vec we get a declared but not defined Werror,
but if we only declare it we get a defined but not used Werror.  We need
to this change to tcg_out_movi eventually anyway, so it's no biggie.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>

---
 tcg/ppc/tcg-target.h     | 25 ++++++++++++++++
 tcg/ppc/tcg-target.opc.h |  5 ++++
 tcg/ppc/tcg-target.inc.c | 65 ++++++++++++++++++++++++++++++++++++++--
 3 files changed, 92 insertions(+), 3 deletions(-)
 create mode 100644 tcg/ppc/tcg-target.opc.h

-- 
2.17.1

Comments

Aleksandar Markovic June 30, 2019, 9:46 a.m. UTC | #1
On Saturday, June 29, 2019, Richard Henderson <richard.henderson@linaro.org>
wrote:

> Introduce all of the flags required to enable tcg backend vector support,

> and a runtime flag to indicate the host supports Altivec instructions.

>

>

If two flags have different purpose and usage, it is better that they
have different
names. (perhaps one of them should have the suffix “_runtime“)

Also, I am not sure if Altiveec can be reffered as isa, it is a
part/extension of an isa, so “isa” seems  superfluous here.

checkpatch warning should also be honored.


> For now, do not actually set have_isa_altivec to true, because we have not

> yet added all of the code to actually generate all of the required insns.

> However, we must define these flags in order to disable ifndefs that create

> stub versions of the functions added here.

>

> The change to tcg_out_movi works around a buglet in tcg.c wherein if we

> do not define tcg_out_dupi_vec we get a declared but not defined Werror,

> but if we only declare it we get a defined but not used Werror.  We need

> to this change to tcg_out_movi eventually anyway, so it's no biggie.

>

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

> Signed-off-by: Aleksandar Markovic <amarkovic@wavecomp.com>

> ---

>  tcg/ppc/tcg-target.h     | 25 ++++++++++++++++

>  tcg/ppc/tcg-target.opc.h |  5 ++++

>  tcg/ppc/tcg-target.inc.c | 65 ++++++++++++++++++++++++++++++++++++++--

>  3 files changed, 92 insertions(+), 3 deletions(-)

>  create mode 100644 tcg/ppc/tcg-target.opc.h

>

> diff --git a/tcg/ppc/tcg-target.h b/tcg/ppc/tcg-target.h

> index 690fa744e1..f6283f468b 100644

> --- a/tcg/ppc/tcg-target.h

> +++ b/tcg/ppc/tcg-target.h

> @@ -58,6 +58,7 @@ typedef enum {

>      TCG_AREG0 = TCG_REG_R27

>  } TCGReg;

>

> +extern bool have_isa_altivec;

>  extern bool have_isa_2_06;

>  extern bool have_isa_3_00;

>

> @@ -135,6 +136,30 @@ extern bool have_isa_3_00;

>  #define TCG_TARGET_HAS_mulsh_i64        1

>  #endif

>

> +/*

> + * While technically Altivec could support V64, it has no 64-bit store

> + * instruction and substituting two 32-bit stores makes the generated

> + * code quite large.

> + */

> +#define TCG_TARGET_HAS_v64              0

> +#define TCG_TARGET_HAS_v128             have_isa_altivec

> +#define TCG_TARGET_HAS_v256             0

> +

> +#define TCG_TARGET_HAS_andc_vec         0

> +#define TCG_TARGET_HAS_orc_vec          0

> +#define TCG_TARGET_HAS_not_vec          0

> +#define TCG_TARGET_HAS_neg_vec          0

> +#define TCG_TARGET_HAS_abs_vec          0

> +#define TCG_TARGET_HAS_shi_vec          0

> +#define TCG_TARGET_HAS_shs_vec          0

> +#define TCG_TARGET_HAS_shv_vec          0

> +#define TCG_TARGET_HAS_cmp_vec          0

> +#define TCG_TARGET_HAS_mul_vec          0

> +#define TCG_TARGET_HAS_sat_vec          0

> +#define TCG_TARGET_HAS_minmax_vec       0

> +#define TCG_TARGET_HAS_bitsel_vec       0

> +#define TCG_TARGET_HAS_cmpsel_vec       0

> +

>  void flush_icache_range(uintptr_t start, uintptr_t stop);

>  void tb_target_set_jmp_target(uintptr_t, uintptr_t, uintptr_t);

>

> diff --git a/tcg/ppc/tcg-target.opc.h b/tcg/ppc/tcg-target.opc.h

> new file mode 100644

> index 0000000000..fa680dd6a0

> --- /dev/null

> +++ b/tcg/ppc/tcg-target.opc.h

> @@ -0,0 +1,5 @@

> +/*

> + * Target-specific opcodes for host vector expansion.  These will be

> + * emitted by tcg_expand_vec_op.  For those familiar with GCC internals,

> + * consider these to be UNSPEC with names.

> + */

> diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c

> index cfbd7ff12c..b938e9aac5 100644

> --- a/tcg/ppc/tcg-target.inc.c

> +++ b/tcg/ppc/tcg-target.inc.c

> @@ -64,6 +64,7 @@

>

>  static tcg_insn_unit *tb_ret_addr;

>

> +bool have_isa_altivec;

>  bool have_isa_2_06;

>  bool have_isa_3_00;

>

> @@ -717,10 +718,31 @@ static void tcg_out_movi_int(TCGContext *s, TCGType

> type, TCGReg ret,

>      }

>  }

>

> -static inline void tcg_out_movi(TCGContext *s, TCGType type, TCGReg ret,

> -                                tcg_target_long arg)

> +static void tcg_out_dupi_vec(TCGContext *s, TCGType type, TCGReg ret,

> +                             tcg_target_long val)

>  {

> -    tcg_out_movi_int(s, type, ret, arg, false);

> +    g_assert_not_reached();

> +}

> +

> +static void tcg_out_movi(TCGContext *s, TCGType type, TCGReg ret,

> +                         tcg_target_long arg)

> +{

> +    switch (type) {

> +    case TCG_TYPE_I32:

> +    case TCG_TYPE_I64:

> +        tcg_debug_assert(ret < TCG_REG_V0);

> +        tcg_out_movi_int(s, type, ret, arg, false);

> +        break;

> +

> +    case TCG_TYPE_V64:

> +    case TCG_TYPE_V128:

> +        tcg_debug_assert(ret >= TCG_REG_V0);

> +        tcg_out_dupi_vec(s, type, ret, arg);

> +        break;

> +

> +    default:

> +        g_assert_not_reached();

> +    }

>  }

>

>  static bool mask_operand(uint32_t c, int *mb, int *me)

> @@ -2605,6 +2627,36 @@ static void tcg_out_op(TCGContext *s, TCGOpcode

> opc, const TCGArg *args,

>      }

>  }

>

> +int tcg_can_emit_vec_op(TCGOpcode opc, TCGType type, unsigned vece)

> +{

> +    g_assert_not_reached();

> +}

> +

> +static bool tcg_out_dup_vec(TCGContext *s, TCGType type, unsigned vece,

> +                            TCGReg dst, TCGReg src)

> +{

> +    g_assert_not_reached();

> +}

> +

> +static bool tcg_out_dupm_vec(TCGContext *s, TCGType type, unsigned vece,

> +                             TCGReg out, TCGReg base, intptr_t offset)

> +{

> +    g_assert_not_reached();

> +}

> +

> +static void tcg_out_vec_op(TCGContext *s, TCGOpcode opc,

> +                           unsigned vecl, unsigned vece,

> +                           const TCGArg *args, const int *const_args)

> +{

> +    g_assert_not_reached();

> +}

> +

> +void tcg_expand_vec_op(TCGOpcode opc, TCGType type, unsigned vece,

> +                       TCGArg a0, ...)

> +{

> +    g_assert_not_reached();

> +}

> +

>  static const TCGTargetOpDef *tcg_target_op_def(TCGOpcode op)

>  {

>      static const TCGTargetOpDef r = { .args_ct_str = { "r" } };

> @@ -2787,6 +2839,9 @@ static void tcg_target_init(TCGContext *s)

>      unsigned long hwcap = qemu_getauxval(AT_HWCAP);

>      unsigned long hwcap2 = qemu_getauxval(AT_HWCAP2);

>

> +    if (hwcap & /* PPC_FEATURE_HAS_ALTIVEC -- NOT YET */ 0) {

> +        have_isa_altivec = true;

> +    }

>      if (hwcap & PPC_FEATURE_ARCH_2_06) {

>          have_isa_2_06 = true;

>      }

> @@ -2798,6 +2853,10 @@ static void tcg_target_init(TCGContext *s)

>

>      tcg_target_available_regs[TCG_TYPE_I32] = 0xffffffff;

>      tcg_target_available_regs[TCG_TYPE_I64] = 0xffffffff;

> +    if (have_isa_altivec) {

> +        tcg_target_available_regs[TCG_TYPE_V64] = 0xffffffff00000000ull;

> +        tcg_target_available_regs[TCG_TYPE_V128] = 0xffffffff00000000ull;

> +    }

>

>      tcg_target_call_clobber_regs = 0;

>      tcg_regset_set_reg(tcg_target_call_clobber_regs, TCG_REG_R0);

> --

> 2.17.1

>

>

>
Richard Henderson June 30, 2019, 10:48 a.m. UTC | #2
On 6/30/19 11:46 AM, Aleksandar Markovic wrote:
> 

> 

> On Saturday, June 29, 2019, Richard Henderson <richard.henderson@linaro.org

> <mailto:richard.henderson@linaro.org>> wrote:

> 

>     Introduce all of the flags required to enable tcg backend vector support,

>     and a runtime flag to indicate the host supports Altivec instructions.

> 

> 

> If two flags have different purpose and usage, it is better that they

> have different names. (perhaps one of them should have the suffix “_runtime“)


Huh?  They do have different names.  Very different names.

> Also, I am not sure if Altiveec can be reffered as isa, it is a part/extension

> of an isa, so “isa” seems  superfluous here.


It also matches the other existing names, so I'll leave it as is.

> checkpatch warning should also be honored.


It's bogus.

> WARNING: Block comments use a leading /* on a separate line

> #155: FILE: tcg/ppc/tcg-target.inc.c:2842:

> +    if (hwcap & /* PPC_FEATURE_HAS_ALTIVEC -- NOT YET */ 0) {


It's not a block comment; the whole thing is on one line.
I have no idea why it doesn't notice.

In any case, this goes away in patch 13.


r~
Aleksandar Markovic June 30, 2019, 11:45 a.m. UTC | #3
On Jun 30, 2019 12:48 PM, "Richard Henderson" <richard.henderson@linaro.org>
wrote:
>

> On 6/30/19 11:46 AM, Aleksandar Markovic wrote:

> >

> >

> > On Saturday, June 29, 2019, Richard Henderson <

richard.henderson@linaro.org
> > <mailto:richard.henderson@linaro.org>> wrote:

> >

> >     Introduce all of the flags required to enable tcg backend vector

support,
> >     and a runtime flag to indicate the host supports Altivec

instructions.
> >

> >

> > If two flags have different purpose and usage, it is better that they

> > have different names. (perhaps one of them should have the suffix

“_runtime“)
>

> Huh?  They do have different names.  Very different names.

>


They do. If you leave the same name, you would make any search for that
name during future debugging/development more difficult.

> > Also, I am not sure if Altiveec can be reffered as isa, it is a

part/extension
> > of an isa, so “isa” seems  superfluous here.

>

> It also matches the other existing names, so I'll leave it as is.

>


If something is wrong in the old code, it does not mean one should continue
the same practice.

> > checkpatch warning should also be honored.

>

> It's bogus.


I don't think it is bogus. The comment should be converted to a regular
one-line ot perhaps multiline comment before if-statement. Althoug it may
be correct in the sense of C-syntax, noone expects coment to be inlined
into if-condition, and it makes the code feel obfuscated, rather than clear.

> > WARNING: Block comments use a leading /* on a separate line

> > #155: FILE: tcg/ppc/tcg-target.inc.c:2842:

> > +    if (hwcap & /* PPC_FEATURE_HAS_ALTIVEC -- NOT YET */ 0) {

>

> It's not a block comment; the whole thing is on one line.

> I have no idea why it doesn't notice.

>

> In any case, this goes away in patch 13.

>

>

> r~
diff mbox series

Patch

diff --git a/tcg/ppc/tcg-target.h b/tcg/ppc/tcg-target.h
index 690fa744e1..f6283f468b 100644
--- a/tcg/ppc/tcg-target.h
+++ b/tcg/ppc/tcg-target.h
@@ -58,6 +58,7 @@  typedef enum {
     TCG_AREG0 = TCG_REG_R27
 } TCGReg;
 
+extern bool have_isa_altivec;
 extern bool have_isa_2_06;
 extern bool have_isa_3_00;
 
@@ -135,6 +136,30 @@  extern bool have_isa_3_00;
 #define TCG_TARGET_HAS_mulsh_i64        1
 #endif
 
+/*
+ * While technically Altivec could support V64, it has no 64-bit store
+ * instruction and substituting two 32-bit stores makes the generated
+ * code quite large.
+ */
+#define TCG_TARGET_HAS_v64              0
+#define TCG_TARGET_HAS_v128             have_isa_altivec
+#define TCG_TARGET_HAS_v256             0
+
+#define TCG_TARGET_HAS_andc_vec         0
+#define TCG_TARGET_HAS_orc_vec          0
+#define TCG_TARGET_HAS_not_vec          0
+#define TCG_TARGET_HAS_neg_vec          0
+#define TCG_TARGET_HAS_abs_vec          0
+#define TCG_TARGET_HAS_shi_vec          0
+#define TCG_TARGET_HAS_shs_vec          0
+#define TCG_TARGET_HAS_shv_vec          0
+#define TCG_TARGET_HAS_cmp_vec          0
+#define TCG_TARGET_HAS_mul_vec          0
+#define TCG_TARGET_HAS_sat_vec          0
+#define TCG_TARGET_HAS_minmax_vec       0
+#define TCG_TARGET_HAS_bitsel_vec       0
+#define TCG_TARGET_HAS_cmpsel_vec       0
+
 void flush_icache_range(uintptr_t start, uintptr_t stop);
 void tb_target_set_jmp_target(uintptr_t, uintptr_t, uintptr_t);
 
diff --git a/tcg/ppc/tcg-target.opc.h b/tcg/ppc/tcg-target.opc.h
new file mode 100644
index 0000000000..fa680dd6a0
--- /dev/null
+++ b/tcg/ppc/tcg-target.opc.h
@@ -0,0 +1,5 @@ 
+/*
+ * Target-specific opcodes for host vector expansion.  These will be
+ * emitted by tcg_expand_vec_op.  For those familiar with GCC internals,
+ * consider these to be UNSPEC with names.
+ */
diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
index cfbd7ff12c..b938e9aac5 100644
--- a/tcg/ppc/tcg-target.inc.c
+++ b/tcg/ppc/tcg-target.inc.c
@@ -64,6 +64,7 @@ 
 
 static tcg_insn_unit *tb_ret_addr;
 
+bool have_isa_altivec;
 bool have_isa_2_06;
 bool have_isa_3_00;
 
@@ -717,10 +718,31 @@  static void tcg_out_movi_int(TCGContext *s, TCGType type, TCGReg ret,
     }
 }
 
-static inline void tcg_out_movi(TCGContext *s, TCGType type, TCGReg ret,
-                                tcg_target_long arg)
+static void tcg_out_dupi_vec(TCGContext *s, TCGType type, TCGReg ret,
+                             tcg_target_long val)
 {
-    tcg_out_movi_int(s, type, ret, arg, false);
+    g_assert_not_reached();
+}
+
+static void tcg_out_movi(TCGContext *s, TCGType type, TCGReg ret,
+                         tcg_target_long arg)
+{
+    switch (type) {
+    case TCG_TYPE_I32:
+    case TCG_TYPE_I64:
+        tcg_debug_assert(ret < TCG_REG_V0);
+        tcg_out_movi_int(s, type, ret, arg, false);
+        break;
+
+    case TCG_TYPE_V64:
+    case TCG_TYPE_V128:
+        tcg_debug_assert(ret >= TCG_REG_V0);
+        tcg_out_dupi_vec(s, type, ret, arg);
+        break;
+
+    default:
+        g_assert_not_reached();
+    }
 }
 
 static bool mask_operand(uint32_t c, int *mb, int *me)
@@ -2605,6 +2627,36 @@  static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg *args,
     }
 }
 
+int tcg_can_emit_vec_op(TCGOpcode opc, TCGType type, unsigned vece)
+{
+    g_assert_not_reached();
+}
+
+static bool tcg_out_dup_vec(TCGContext *s, TCGType type, unsigned vece,
+                            TCGReg dst, TCGReg src)
+{
+    g_assert_not_reached();
+}
+
+static bool tcg_out_dupm_vec(TCGContext *s, TCGType type, unsigned vece,
+                             TCGReg out, TCGReg base, intptr_t offset)
+{
+    g_assert_not_reached();
+}
+
+static void tcg_out_vec_op(TCGContext *s, TCGOpcode opc,
+                           unsigned vecl, unsigned vece,
+                           const TCGArg *args, const int *const_args)
+{
+    g_assert_not_reached();
+}
+
+void tcg_expand_vec_op(TCGOpcode opc, TCGType type, unsigned vece,
+                       TCGArg a0, ...)
+{
+    g_assert_not_reached();
+}
+
 static const TCGTargetOpDef *tcg_target_op_def(TCGOpcode op)
 {
     static const TCGTargetOpDef r = { .args_ct_str = { "r" } };
@@ -2787,6 +2839,9 @@  static void tcg_target_init(TCGContext *s)
     unsigned long hwcap = qemu_getauxval(AT_HWCAP);
     unsigned long hwcap2 = qemu_getauxval(AT_HWCAP2);
 
+    if (hwcap & /* PPC_FEATURE_HAS_ALTIVEC -- NOT YET */ 0) {
+        have_isa_altivec = true;
+    }
     if (hwcap & PPC_FEATURE_ARCH_2_06) {
         have_isa_2_06 = true;
     }
@@ -2798,6 +2853,10 @@  static void tcg_target_init(TCGContext *s)
 
     tcg_target_available_regs[TCG_TYPE_I32] = 0xffffffff;
     tcg_target_available_regs[TCG_TYPE_I64] = 0xffffffff;
+    if (have_isa_altivec) {
+        tcg_target_available_regs[TCG_TYPE_V64] = 0xffffffff00000000ull;
+        tcg_target_available_regs[TCG_TYPE_V128] = 0xffffffff00000000ull;
+    }
 
     tcg_target_call_clobber_regs = 0;
     tcg_regset_set_reg(tcg_target_call_clobber_regs, TCG_REG_R0);