Re: Re: [PATCH] RISC-V: Support LEN_FOLD_EXTRACT_LAST auto-vectorization
>> The use_real_merge just appeared odd to me here because there is
>> nothing to merge. But in the end it's just to omit the vundef operand
>> so good for now. There is an increasing number of opportunities to
>> refactor in riscv-v.cc, though ;)
I think we can change use_real_merge into use_dummy_merge?
When it's true then add undef merge :
if (!m_use_real_merge_p)
add_vundef_operand ();
change it into:
if (m_use_dummy_merge_p)
add_vundef_operand ();
Then we can avoid the confusion.
juzhe.zh...@rivai.ai
From: Robin Dapp
Date: 2023-08-24 17:13
To: Juzhe-Zhong; gcc-patches
CC: rdapp.gcc; kito.cheng; kito.cheng; jeffreyalaw
Subject: Re: [PATCH] RISC-V: Support LEN_FOLD_EXTRACT_LAST auto-vectorization
Hi Juzhe,
> vcpop.m a5,v0
> beq a5,zero,.L3
> addi a5,a5,-1
> vsetvli a4,zero,e32,m1,ta,ma
> vcompress.vm v2,v3,v0
> vslidedown.vx v2,v2,a5
> vmv.x.s a0,v2
> .L3:
> sext.w a0,a0
Mhm, where is this sext coming from? Thought I had this covered with
the autovec-opt pattern but apparently not. I'll take that, nothing
related to this patch.
> --- a/gcc/config/riscv/riscv-v.cc
> +++ b/gcc/config/riscv/riscv-v.cc
> @@ -213,7 +213,7 @@ public:
>{
> /* Optimize VLS-VLMAX code gen, we can use vsetivli instead of
> the vsetvli to obtain the value of vlmax. */
> - poly_uint64 nunits = GET_MODE_NUNITS (m_dest_mode);
> + poly_uint64 nunits = GET_MODE_NUNITS (m_mask_mode);
Why is that necessary? Just for the popcount I presume?
Can't we rather have a new case for a scalar destination? I find
the code a bit misleading now as we check m_dest_mode and then not
use it.
>
> +/* Emit vcpop.m instruction. */
> +
> +static void
> +emit_cpop_insn (unsigned icode, rtx *ops, rtx len)
> +{
> + machine_mode dest_mode = GET_MODE (ops[0]);
> + machine_mode mask_mode = GET_MODE (ops[1]);
> + insn_expander e (RVV_CPOP,
> + /* HAS_DEST_P */ true,
> + /* FULLY_UNMASKED_P */ true,
> + /* USE_REAL_MERGE_P */ true,
> + /* HAS_AVL_P */ true,
> + /* VLMAX_P */ len ? false : true,
> + dest_mode, mask_mode);
> +
> + e.set_vl (len);
> + e.emit_insn ((enum insn_code) icode, ops);
> +}
The use_real_merge just appeared odd to me here because there is
nothing to merge. But in the end it's just to omit the vundef operand
so good for now. There is an increasing number of opportunities to
refactor in riscv-v.cc, though ;)
The rest looks good to me. Note that my machine crashed when
compiling the extract_last-14.c because it used up all my RAM.
The vsetvl "refactor" phase 3 patch helped, though.
We'd need to have this patch depend on the other one then.
The rest looks good to me. At first I was a bit wary about the
branching zero check after popcount but as we're outside of a loop
anyway, that's fine. Might want to use a conditional select in the
future but actually not that important.
Regards
Robin
Re: Re: [PATCH] RISC-V: Support LEN_FOLD_EXTRACT_LAST auto-vectorization
>> Why is that necessary? Just for the popcount I presume?
>> Can't we rather have a new case for a scalar destination? I find
>> the code a bit misleading now as we check m_dest_mode and then not
>> use it.
I am gonna fix it in V2.
>> The rest looks good to me. Note that my machine crashed when
>> compiling the extract_last-14.c because it used up all my RAM.
>> The vsetvl "refactor" phase 3 patch helped, though.
>> We'd need to have this patch depend on the other one then.
Yes. The refactor patch fixed potential bugs. I will commit that tomorrow
when kito no more comments.
juzhe.zh...@rivai.ai
From: Robin Dapp
Date: 2023-08-24 17:13
To: Juzhe-Zhong; gcc-patches
CC: rdapp.gcc; kito.cheng; kito.cheng; jeffreyalaw
Subject: Re: [PATCH] RISC-V: Support LEN_FOLD_EXTRACT_LAST auto-vectorization
Hi Juzhe,
> vcpop.m a5,v0
> beq a5,zero,.L3
> addi a5,a5,-1
> vsetvli a4,zero,e32,m1,ta,ma
> vcompress.vm v2,v3,v0
> vslidedown.vx v2,v2,a5
> vmv.x.s a0,v2
> .L3:
> sext.w a0,a0
Mhm, where is this sext coming from? Thought I had this covered with
the autovec-opt pattern but apparently not. I'll take that, nothing
related to this patch.
> --- a/gcc/config/riscv/riscv-v.cc
> +++ b/gcc/config/riscv/riscv-v.cc
> @@ -213,7 +213,7 @@ public:
>{
> /* Optimize VLS-VLMAX code gen, we can use vsetivli instead of
> the vsetvli to obtain the value of vlmax. */
> - poly_uint64 nunits = GET_MODE_NUNITS (m_dest_mode);
> + poly_uint64 nunits = GET_MODE_NUNITS (m_mask_mode);
Why is that necessary? Just for the popcount I presume?
Can't we rather have a new case for a scalar destination? I find
the code a bit misleading now as we check m_dest_mode and then not
use it.
>
> +/* Emit vcpop.m instruction. */
> +
> +static void
> +emit_cpop_insn (unsigned icode, rtx *ops, rtx len)
> +{
> + machine_mode dest_mode = GET_MODE (ops[0]);
> + machine_mode mask_mode = GET_MODE (ops[1]);
> + insn_expander e (RVV_CPOP,
> + /* HAS_DEST_P */ true,
> + /* FULLY_UNMASKED_P */ true,
> + /* USE_REAL_MERGE_P */ true,
> + /* HAS_AVL_P */ true,
> + /* VLMAX_P */ len ? false : true,
> + dest_mode, mask_mode);
> +
> + e.set_vl (len);
> + e.emit_insn ((enum insn_code) icode, ops);
> +}
The use_real_merge just appeared odd to me here because there is
nothing to merge. But in the end it's just to omit the vundef operand
so good for now. There is an increasing number of opportunities to
refactor in riscv-v.cc, though ;)
The rest looks good to me. Note that my machine crashed when
compiling the extract_last-14.c because it used up all my RAM.
The vsetvl "refactor" phase 3 patch helped, though.
We'd need to have this patch depend on the other one then.
The rest looks good to me. At first I was a bit wary about the
branching zero check after popcount but as we're outside of a loop
anyway, that's fine. Might want to use a conditional select in the
future but actually not that important.
Regards
Robin
Re: [PATCH] RISC-V: Support LEN_FOLD_EXTRACT_LAST auto-vectorization
Hi Juzhe,
> vcpop.m a5,v0
> beq a5,zero,.L3
> addia5,a5,-1
> vsetvli a4,zero,e32,m1,ta,ma
> vcompress.vmv2,v3,v0
> vslidedown.vx v2,v2,a5
> vmv.x.s a0,v2
> .L3:
> sext.w a0,a0
Mhm, where is this sext coming from? Thought I had this covered with
the autovec-opt pattern but apparently not. I'll take that, nothing
related to this patch.
> --- a/gcc/config/riscv/riscv-v.cc
> +++ b/gcc/config/riscv/riscv-v.cc
> @@ -213,7 +213,7 @@ public:
> {
> /* Optimize VLS-VLMAX code gen, we can use vsetivli instead of
> the vsetvli to obtain the value of vlmax. */
> - poly_uint64 nunits = GET_MODE_NUNITS (m_dest_mode);
> + poly_uint64 nunits = GET_MODE_NUNITS (m_mask_mode);
Why is that necessary? Just for the popcount I presume?
Can't we rather have a new case for a scalar destination? I find
the code a bit misleading now as we check m_dest_mode and then not
use it.
>
> +/* Emit vcpop.m instruction. */
> +
> +static void
> +emit_cpop_insn (unsigned icode, rtx *ops, rtx len)
> +{
> + machine_mode dest_mode = GET_MODE (ops[0]);
> + machine_mode mask_mode = GET_MODE (ops[1]);
> + insn_expander e (RVV_CPOP,
> + /* HAS_DEST_P */ true,
> + /* FULLY_UNMASKED_P */ true,
> + /* USE_REAL_MERGE_P */ true,
> + /* HAS_AVL_P */ true,
> + /* VLMAX_P */ len ? false : true,
> + dest_mode, mask_mode);
> +
> + e.set_vl (len);
> + e.emit_insn ((enum insn_code) icode, ops);
> +}
The use_real_merge just appeared odd to me here because there is
nothing to merge. But in the end it's just to omit the vundef operand
so good for now. There is an increasing number of opportunities to
refactor in riscv-v.cc, though ;)
The rest looks good to me. Note that my machine crashed when
compiling the extract_last-14.c because it used up all my RAM.
The vsetvl "refactor" phase 3 patch helped, though.
We'd need to have this patch depend on the other one then.
The rest looks good to me. At first I was a bit wary about the
branching zero check after popcount but as we're outside of a loop
anyway, that's fine. Might want to use a conditional select in the
future but actually not that important.
Regards
Robin
