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

[Richard Henderson]

On 1/4/24 00:21, Philippe Mathieu-Daudé wrote:
> 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?

Yes, you're right.  There's some logical overlap between tcg_gen_emit_vec_op and 
tcg_op_supported, and I think I confused myself a bit there.


r~