Re: [PATCH 04/20] tcg/s390x: Implement vector NAND, NOR, EQV

[Philippe Mathieu-Daudé]

Hi Richard,

(revisiting this old patch which is now commit 21eab5bfae)

On 18/12/21 20:42, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   tcg/s390x/tcg-target.h     |  6 +++---
>   tcg/s390x/tcg-target.c.inc | 17 +++++++++++++++++
>   2 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/tcg/s390x/tcg-target.h b/tcg/s390x/tcg-target.h
> index ad29e62b16..fef227b0fe 100644
> --- a/tcg/s390x/tcg-target.h
> +++ b/tcg/s390x/tcg-target.h
> @@ -145,9 +145,9 @@ extern uint64_t s390_facilities[3];
>   
>   #define TCG_TARGET_HAS_andc_vec       1
>   #define TCG_TARGET_HAS_orc_vec        HAVE_FACILITY(VECTOR_ENH1)
> -#define TCG_TARGET_HAS_nand_vec       0
> -#define TCG_TARGET_HAS_nor_vec        0
> -#define TCG_TARGET_HAS_eqv_vec        0
> +#define TCG_TARGET_HAS_nand_vec       HAVE_FACILITY(VECTOR_ENH1)
> +#define TCG_TARGET_HAS_nor_vec        1
> +#define TCG_TARGET_HAS_eqv_vec        HAVE_FACILITY(VECTOR_ENH1)

Here some opcodes are conditional, ...

>   #define TCG_TARGET_HAS_not_vec        1
>   #define TCG_TARGET_HAS_neg_vec        1
>   #define TCG_TARGET_HAS_abs_vec        1
> diff --git a/tcg/s390x/tcg-target.c.inc b/tcg/s390x/tcg-target.c.inc
> index 57e803e339..5a90b892cb 100644
> --- a/tcg/s390x/tcg-target.c.inc
> +++ b/tcg/s390x/tcg-target.c.inc
> @@ -288,7 +288,9 @@ typedef enum S390Opcode {
>       VRRc_VMXL   = 0xe7fd,
>       VRRc_VN     = 0xe768,
>       VRRc_VNC    = 0xe769,
> +    VRRc_VNN    = 0xe76e,
>       VRRc_VNO    = 0xe76b,
> +    VRRc_VNX    = 0xe76c,
>       VRRc_VO     = 0xe76a,
>       VRRc_VOC    = 0xe76f,
>       VRRc_VPKS   = 0xe797,   /* we leave the m5 cs field 0 */
> @@ -2750,6 +2752,15 @@ static void tcg_out_vec_op(TCGContext *s, TCGOpcode opc,
>       case INDEX_op_xor_vec:
>           tcg_out_insn(s, VRRc, VX, a0, a1, a2, 0);
>           break;
> +    case INDEX_op_nand_vec:
> +        tcg_out_insn(s, VRRc, VNN, a0, a1, a2, 0);
> +        break;
> +    case INDEX_op_nor_vec:
> +        tcg_out_insn(s, VRRc, VNO, a0, a1, a2, 0);
> +        break;
> +    case INDEX_op_eqv_vec:
> +        tcg_out_insn(s, VRRc, VNX, a0, a1, a2, 0);
> +        break;
>   
>       case INDEX_op_shli_vec:
>           tcg_out_insn(s, VRSa, VESL, a0, a2, TCG_REG_NONE, a1, vece);
> @@ -2846,7 +2857,10 @@ int tcg_can_emit_vec_op(TCGOpcode opc, TCGType type, 
> unsigned vece)
>       case INDEX_op_and_vec:
>       case INDEX_op_andc_vec:
>       case INDEX_op_bitsel_vec:
> +    case INDEX_op_eqv_vec:
> +    case INDEX_op_nand_vec:

... but here we unconditionally return 1 for them.

Shouldn't we return TCG_TARGET_HAS_opcode instead?

>       case INDEX_op_neg_vec:
> +    case INDEX_op_nor_vec:
>       case INDEX_op_not_vec:
>       case INDEX_op_or_vec:
>       case INDEX_op_orc_vec:

(expanding context)

            return 1;

> @@ -3191,6 +3205,9 @@ static TCGConstraintSetIndex tcg_target_op_def(TCGOpcode 
> op)
>       case INDEX_op_or_vec:
>       case INDEX_op_orc_vec:
>       case INDEX_op_xor_vec:
> +    case INDEX_op_nand_vec:
> +    case INDEX_op_nor_vec:
> +    case INDEX_op_eqv_vec:
>       case INDEX_op_cmp_vec:
>       case INDEX_op_mul_vec:
>       case INDEX_op_rotlv_vec: