>> Any specific reason for MAX_BITS_PER_WORD instead of
>> GET_MODE_BITSIZE (Pmode)? In general I like the idea to switch
>> to scalar moves here but couldn't it already be debatable for
>> a 64-bit move on rv32 i.e. a question of costing?
V2SImode for example,
I think
I prefer this following sequence:
lw
lw
sw
sw
instead of:
vsetvli zero, 2, e32, mf2
vle
vse
in RV32 system.
>> Here as well as before, why the assert? Is this intended for
>> easier debugging later?
Since before may be false, wheras now it should always be true for VLS modes.
>> These are the same as for the fractional modes but we define
>> the expanders for all modes instead. legitimize_move will only
>> create the _lra if the mode is indeed smaller than a vector and
>> I assume the scratch will prevent us from ever generating the
>> insn any other way. Still, couldn't we add the new modes to
>> V_FRACT? Or didn't you want to mix VLA and VLS modes there?
>> The others seem to be mixed, though.
No, you should take a look at the splitting, VLSmodes are different from
V_FRACT.
>> What do we need this for?
>> Maybe for this? Won't MIN suffice?
Address comments.
>> Why do these iterator modes have a condition on VLS while the following
>> ones don't? It's probably not terribly important as it works either way
>> but still sticks out.
Address comments.
>> Btw. as a general remark. In the past I also found the single-element
>> vectors helpful for codegen but that might be obsolete. Not in scope
>> for this patch.
Address comments. I saw many targets added single-element vector.
I will add it in V2.
juzhe.zh...@rivai.ai
From: Robin Dapp
Date: 2023-07-27 04:27
To: Juzhe-Zhong; gcc-patches
CC: rdapp.gcc; kito.cheng; kito.cheng; jeffreyalaw
Subject: Re: [PATCH] RISC-V: Enable basic VLS modes support
Hi Juzhe,
just some small remarks, all in all no major concerns.
> + vmv%m1r.v\t%0,%1"
> + "&& (!register_operand (operands[0], <MODE>mode)
> + || !register_operand (operands[1], <MODE>mode))"
> + [(const_int 0)]
> + {
> + unsigned size = GET_MODE_BITSIZE (<MODE>mode).to_constant ();
> + if (size <= MAX_BITS_PER_WORD
Any specific reason for MAX_BITS_PER_WORD instead of
GET_MODE_BITSIZE (Pmode)? In general I like the idea to switch
to scalar moves here but couldn't it already be debatable for
a 64-bit move on rv32 i.e. a question of costing?
> + gcc_assert (ok_p);
> + DONE;
Here as well as before, why the assert? Is this intended for
easier debugging later?
> +
> +(define_expand "@mov<VLS_AVL_REG:mode><P:mode>_lra"
> + [(parallel
> + [(set (match_operand:VLS_AVL_REG 0 "reg_or_mem_operand")
> + (match_operand:VLS_AVL_REG 1 "reg_or_mem_operand"))
> + (clobber (match_scratch:P 2))])]
> + "TARGET_VECTOR && (lra_in_progress || reload_completed)"
> +{})
> +
> +(define_insn_and_split "*mov<VLS_AVL_REG:mode><P:mode>_lra"
These are the same as for the fractional modes but we define
the expanders for all modes instead. legitimize_move will only
create the _lra if the mode is indeed smaller than a vector and
I assume the scratch will prevent us from ever generating the
insn any other way. Still, couldn't we add the new modes to
V_FRACT? Or didn't you want to mix VLA and VLS modes there?
The others seem to be mixed, though.
> +#define INCLUDE_ALGORITHM
What do we need this for?
> + int inner_size = GET_MODE_BITSIZE (GET_MODE_INNER (mode));
> + if (size < TARGET_MIN_VLEN)
> + {
> + int factor = TARGET_MIN_VLEN / size;
> + if (inner_size == 8)
> + factor = std::min (factor, 8);
Maybe for this? Won't MIN suffice?
> + ;; VLS modes.
> + (V2QI "TARGET_VECTOR_VLS")
Why do these iterator modes have a condition on VLS while the following
ones don't? It's probably not terribly important as it works either way
but still sticks out.
Btw. as a general remark. In the past I also found the single-element
vectors helpful for codegen but that might be obsolete. Not in scope
for this patch.
Regards
Robin
