Re: [PATCH] RISC-V: Implement autovec abs, vneg, vnot.
> > So I expect you will also apply those refactor on Juzhe's new changes? > > If so I would like to have a separated NFC refactor patch if possible. > > What's NFC? :) Do you mean to just have the refactor part as a separate > patch? If yes, I agree. NFC: non-functional-change, that's a term used in LLVM, I just forgot that's kind of rare term used here,
Re: Re: [PATCH] RISC-V: Implement autovec abs, vneg, vnot.
Yeah, I agree wit kito. For example, I see you have rename "get_prefer_***" into "get_preferred_**" I think this NFC patch should be separated patch. Thanks. juzhe.zh...@rivai.ai From: Kito Cheng Date: 2023-05-22 17:05 To: Robin Dapp CC: 钟居哲; gcc-patches; palmer; Michael Collison; Jeff Law Subject: Re: [PATCH] RISC-V: Implement autovec abs, vneg, vnot. So I expect you will also apply those refactor on Juzhe's new changes? If so I would like to have a separated NFC refactor patch if possible. e.g. Juzhe's vec_cmp/vcond -> NFC refactor patch -> abs, vneg, vnot On Mon, May 22, 2023 at 4:59 PM Robin Dapp wrote: > > As discussed with Juzhe off-list, I will rebase this patch against > Juzhe's vec_cmp/vcond patch once that hits the trunk. > > Regards > Robin
Re: [PATCH] RISC-V: Implement autovec abs, vneg, vnot.
> So I expect you will also apply those refactor on Juzhe's new changes? > If so I would like to have a separated NFC refactor patch if possible. What's NFC? :) Do you mean to just have the refactor part as a separate patch? If yes, I agree. > e.g. > Juzhe's vec_cmp/vcond -> NFC refactor patch -> abs, vneg, vnot
Re: [PATCH] RISC-V: Implement autovec abs, vneg, vnot.
So I expect you will also apply those refactor on Juzhe's new changes? If so I would like to have a separated NFC refactor patch if possible. e.g. Juzhe's vec_cmp/vcond -> NFC refactor patch -> abs, vneg, vnot On Mon, May 22, 2023 at 4:59 PM Robin Dapp wrote: > > As discussed with Juzhe off-list, I will rebase this patch against > Juzhe's vec_cmp/vcond patch once that hits the trunk. > > Regards > Robin
Re: [PATCH] RISC-V: Implement autovec abs, vneg, vnot.
As discussed with Juzhe off-list, I will rebase this patch against Juzhe's vec_cmp/vcond patch once that hits the trunk. Regards Robin
Re: Re: [PATCH] RISC-V: Implement autovec abs, vneg, vnot.
>> What about the rest of the changes? It's not all typos but I tried
>> to unify the mask/policy handling a bit.
Oh, I see. You rename get_prefer into get_preferred.
This makes perfect sense to me.
juzhe.zh...@rivai.ai
From: Robin Dapp
Date: 2023-05-19 20:07
To: 钟居哲; gcc-patches; kito.cheng; palmer; Michael Collison; Jeff Law
CC: rdapp.gcc
Subject: Re: [PATCH] RISC-V: Implement autovec abs, vneg, vnot.
>>> + TAIL_UNDEFINED = -1,
>>> + MASK_UNDEFINED = -1,
> Why you add this ?
>
>>> + void add_policy_operands (enum tail_policy vta = TAIL_UNDEFINED,
>>> + enum mask_policy vma = MASK_UNDEFINED)
> No, you should just specify this as TAIL_ANY or MASK_ANY as default value.
That's the value I intended for "unspecified" i.e. the caller
didn't specify and then set it to the default. _ANY can work as
well I guess.
>
>>>const_vlmax_p (machine_mode mode)
>>>{
>>>- poly_uint64 nuints = GET_MODE_NUNITS (mode);
>>>+ poly_uint64 nunits = GET_MODE_NUNITS (mode);
>>>- return nuints.is_constant ()
>>>+ return nunits.is_constant ()
>>> /* The vsetivli can only hold register 0~31. */
>>>-? (IN_RANGE (nuints.to_constant (), 0, 31))
>>>+? (IN_RANGE (nunits.to_constant (), 0, 31))
>>> /* Only allowed in VLS-VLMAX mode. */
>>> : false;
>>>}
> Meaningless change ?
Typo.
>
>>>/* For the instruction that doesn't require TA, we still need a default
>>> value
>>> to emit vsetvl. We pick up the default value according to prefer
>>> policy. */
>>>- return (bool) (get_prefer_tail_policy () & 0x1
>>>- || (get_prefer_tail_policy () >> 1 & 0x1));
>>>+ return (bool) (get_preferred_tail_policy () & 0x1
>>>+ || (get_preferred_tail_policy () >> 1 & 0x1));
>>>}
>>>/* Get default mask policy. */
>>>@@ -576,8 +576,8 @@ get_default_ma ()
>>>{
>>> /* For the instruction that doesn't require MA, we still need a
>>> default value
>>> to emit vsetvl. We pick up the default value according to prefer
>>> policy. */
>>>- return (bool) (get_prefer_mask_policy () & 0x1
>>>- || (get_prefer_mask_policy () >> 1 & 0x1));
>>>+ return (bool) (get_preferred_mask_policy () & 0x1
>>>+ || (get_preferred_mask_policy () >> 1 & 0x1));
> Why you change it ?
Typo/grammar imho.
What about the rest of the changes? It's not all typos but I tried
to unify the mask/policy handling a bit.
> You are using comparison helper which I added one in my downstream
> when I am working on comparison autovec patterns:
>
> I think you can normalize my code with yours:
I wasn't aware that I'm only using one of several helpers, just refactored
what iss upstream. Yes your code looks reasonable and it surely works
with the patch without much rework.
> I am almost done all comparison autovec patterns, soon will send them after
> testing.
Good, looking forward to it.
Regards
Robin
Re: [PATCH] RISC-V: Implement autovec abs, vneg, vnot.
>>> + TAIL_UNDEFINED = -1,
>>> + MASK_UNDEFINED = -1,
> Why you add this ?
>
>>> + void add_policy_operands (enum tail_policy vta = TAIL_UNDEFINED,
>>> + enum mask_policy vma = MASK_UNDEFINED)
> No, you should just specify this as TAIL_ANY or MASK_ANY as default value.
That's the value I intended for "unspecified" i.e. the caller
didn't specify and then set it to the default. _ANY can work as
well I guess.
>
>>>const_vlmax_p (machine_mode mode)
>>>{
>>>- poly_uint64 nuints = GET_MODE_NUNITS (mode);
>>>+ poly_uint64 nunits = GET_MODE_NUNITS (mode);
>>>- return nuints.is_constant ()
>>>+ return nunits.is_constant ()
>>> /* The vsetivli can only hold register 0~31. */
>>>- ? (IN_RANGE (nuints.to_constant (), 0, 31))
>>>+ ? (IN_RANGE (nunits.to_constant (), 0, 31))
>>> /* Only allowed in VLS-VLMAX mode. */
>>> : false;
>>>}
> Meaningless change ?
Typo.
>
>>> /* For the instruction that doesn't require TA, we still need a default
>>>value
>>> to emit vsetvl. We pick up the default value according to prefer
>>>policy. */
>>> - return (bool) (get_prefer_tail_policy () & 0x1
>>> - || (get_prefer_tail_policy () >> 1 & 0x1));
>>> + return (bool) (get_preferred_tail_policy () & 0x1
>>> + || (get_preferred_tail_policy () >> 1 & 0x1));
>>> }
>>> /* Get default mask policy. */
>>> @@ -576,8 +576,8 @@ get_default_ma ()
>>> {
>>> /* For the instruction that doesn't require MA, we still need a
>>>default value
>>> to emit vsetvl. We pick up the default value according to prefer
>>>policy. */
>>> - return (bool) (get_prefer_mask_policy () & 0x1
>>> - || (get_prefer_mask_policy () >> 1 & 0x1));
>>> + return (bool) (get_preferred_mask_policy () & 0x1
>>> + || (get_preferred_mask_policy () >> 1 & 0x1));
> Why you change it ?
Typo/grammar imho.
What about the rest of the changes? It's not all typos but I tried
to unify the mask/policy handling a bit.
> You are using comparison helper which I added one in my downstream
> when I am working on comparison autovec patterns:
>
> I think you can normalize my code with yours:
I wasn't aware that I'm only using one of several helpers, just refactored
what iss upstream. Yes your code looks reasonable and it surely works
with the patch without much rework.
> I am almost done all comparison autovec patterns, soon will send them after
> testing.
Good, looking forward to it.
Regards
Robin
Re: [PATCH] RISC-V: Implement autovec abs, vneg, vnot.
>> + TAIL_UNDEFINED = -1,
>> + MASK_UNDEFINED = -1,
Why you add this ?
>> + void add_policy_operands (enum tail_policy vta = TAIL_UNDEFINED,
>> + enum mask_policy vma = MASK_UNDEFINED)
No, you should just specify this as TAIL_ANY or MASK_ANY as default value.
>>const_vlmax_p (machine_mode mode)
>>{
>>- poly_uint64 nuints = GET_MODE_NUNITS (mode);
>>+ poly_uint64 nunits = GET_MODE_NUNITS (mode);
>>- return nuints.is_constant ()
>>+ return nunits.is_constant ()
>> /* The vsetivli can only hold register 0~31. */
>>-? (IN_RANGE (nuints.to_constant (), 0, 31))
>>+? (IN_RANGE (nunits.to_constant (), 0, 31))
>> /* Only allowed in VLS-VLMAX mode. */
>> : false;
>>}
Meaningless change ?
>>/* For the instruction that doesn't require TA, we still need a default
>> value
>> to emit vsetvl. We pick up the default value according to prefer
>> policy. */
>>- return (bool) (get_prefer_tail_policy () & 0x1
>>- || (get_prefer_tail_policy () >> 1 & 0x1));
>>+ return (bool) (get_preferred_tail_policy () & 0x1
>>+ || (get_preferred_tail_policy () >> 1 & 0x1));
>>}
>>/* Get default mask policy. */
>>@@ -576,8 +576,8 @@ get_default_ma ()
>>{
>> /* For the instruction that doesn't require MA, we still need a
>> default value
>> to emit vsetvl. We pick up the default value according to prefer
>> policy. */
>>- return (bool) (get_prefer_mask_policy () & 0x1
>>- || (get_prefer_mask_policy () >> 1 & 0x1));
>>+ return (bool) (get_preferred_mask_policy () & 0x1
>>+ || (get_preferred_mask_policy () >> 1 & 0x1));
Why you change it ?
>> +/* Emit an RVV comparison. */
>> +static void
>> +emit_pred_cmp (unsigned icode, rtx mask, rtx dest, rtx cmp,
>> +rtx src1, rtx src2,
>> +rtx len, machine_mode mask_mode)
>> +{
>> + insn_expander<9> e;
>> +
>> + e.set_dest_and_mask (dest, mask, mask_mode);
>> +
>> + e.add_input_operand (cmp, GET_MODE (cmp));
>> +
>> + e.add_source_operand (src1, GET_MODE (src1));
>> + e.add_source_operand (src2, GET_MODE (src2));
You are using comparison helper which I added one in my downstream
when I am working on comparison autovec patterns:
I think you can normalize my code with yours:
/* Emit an RVV comparison. If one of SRC1 and SRC2 is a scalar operand, its
data_mode is specified using SCALAR_MODE. */
static void
emit_pred_comparison (unsigned icode, rtx_code rcode, rtx mask, rtx dest,
rtx src1, rtx src2, rtx len, machine_mode mask_mode,
machine_mode scalar_mode = VOIDmode)
{
insn_expander<9> e;
e.set_dest_and_mask (mask, dest, mask_mode);
machine_mode data_mode = GET_MODE (src1);
gcc_assert (VECTOR_MODE_P (GET_MODE (src1))
|| VECTOR_MODE_P (GET_MODE (src2)));
if (!insn_operand_matches ((enum insn_code) icode, e.opno () + 1, src1))
src1 = force_reg (data_mode, src1);
if (!insn_operand_matches ((enum insn_code) icode, e.opno () + 2, src2))
{
if (VECTOR_MODE_P (GET_MODE (src2)))
src2 = force_reg (data_mode, src2);
else
src2 = force_reg (scalar_mode, src2);
}
rtx comparison = gen_rtx_fmt_ee (rcode, mask_mode, src1, src2);
if (!VECTOR_MODE_P (GET_MODE (src2)))
comparison = gen_rtx_fmt_ee (rcode, mask_mode, src1,
gen_rtx_VEC_DUPLICATE (data_mode, src2));
e.add_fixed_operand (comparison);
e.add_fixed_operand (src1);
if (CONST_INT_P (src2))
e.add_integer_operand (src2);
else
e.add_fixed_operand (src2);
e.set_len_and_policy (len, true, false, true);
e.expand ((enum insn_code) icode, false);
}
static void
emit_len_comparison (unsigned icode, rtx_code rcode, rtx dest, rtx src1,
rtx src2, rtx len, machine_mode mask_mode,
machine_mode scalar_mode)
{
emit_pred_comparison (icode, rcode, NULL_RTX, dest, src1, src2, len,
mask_mode, scalar_mode);
}
/* Expand an RVV integer comparison using the RVV equivalent of:
(set TARGET (CODE OP0 OP1)). */
void
expand_vec_cmp_int (rtx target, rtx_code code, rtx op0, rtx op1)
{
machine_mode mask_mode = GET_MODE (target);
machine_mode data_mode = GET_MODE (op0);
insn_code icode;
bool scalar_p = false;
if (CONST_VECTOR_P (op1))
{
rtx elt;
if (const_vec_duplicate_p (op1, &elt))
op1 = elt;
scalar_p = true;
}
switch (code)
{
case LE:
case LEU:
case GT:
case GTU:
if (scalar_p)
icode = code_for_pred_cmp_scalar (data_mode);
else
icode = code_for_pred_cmp (data_mode);
break;
case EQ:
case NE:
if (scalar_p)
icode = code_for_pred_eqne_scalar (data_mode);
else
icode = code_for_pred_cmp (data_mode);
break;
case LT:
case LTU:
if (scalar_p)
icode = code_for_pred_cmp_scalar (data_mode);
else
icode = code_for_pred_ltge (data_mode);
break;
case GE:
case GEU:
if (scalar_p)
icode = code_for_pred_ge_scalar (data_mode);
else
icode
