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

[Richard Henderson <richard.henderson@linaro.org>]

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~