Ok.  LGTM as long as you change the patch as I suggested.

Thanks.


juzhe.zhong@rivai.ai
 
From: Kito Cheng
Date: 2023-05-30 14:51
To: juzhe.zhong@rivai.ai
CC: gcc-patches; palmer; kito.cheng; jeffreyalaw; Robin Dapp; pan2.li
Subject: Re: [PATCH] RISC-V: Basic VLS code gen for RISC-V
> >> /* Return true if MODE is true VLS mode.  */
> >> bool
> >> vls_mode_p (machine_mode mode)
> >> {
> >>   switch (mode)
> >>     {
> >>     case E_V4SImode:
> >>     case E_V2DImode:
> >>     case E_V8HImode:
> >>     case E_V16QImode:
> >>       return true;
> >>     default:
> >>       return false;
> >>     }
> >> }
>
> To be consistent, you should put these into riscv-vector-switching.def.
> It can make the function easier extend,change it like this:
> change name into riscv_v_ext_vls_mode_p
>
> bool
> riscv_v_ext_vls_mode_p (machine_mode mode)
> {
> #define VLS_ENTRY(MODE, REQUIREMENT, ...)                                          \
>   case MODE##mode:                                                             \
>     return REQUIREMENT;
>   switch (mode)
>     {
> #include "riscv-vector-switch.def"
>     default:
>       return false;
>     }
>   return false;
> }
>
> Then in riscv-vector-switch.def
> VLS_ENTRY (V4SI...
> VLS_ENTRY (V2DI..
> ...
> In the future, we extend more VLS modes in riscv-vector-switch.def
 
Good point, we should make this more consistent :)
 
> >>(define_insn_and_split "<optab><mode>3"
> >>  [(set (match_operand:VLS 0 "register_operand" "=vr")
> >> (any_int_binop_no_shift:VLS
> >>  (match_operand:VLS 1 "register_operand" "vr")
> >>  (match_operand:VLS 2 "register_operand" "vr")))]
> >>  "TARGET_VECTOR"
> >>  "#"
> >>  "reload_completed"
> >>  [(const_int 0)]
> >>+{
> >>  machine_mode vla_mode = riscv_vector::minimal_vla_mode (<MODE>mode);
> >>  riscv_vector::vls_insn_expander (
> >>    code_for_pred (<CODE>, vla_mode), riscv_vector::RVV_BINOP,
> >>    operands, <MODE>mode, vla_mode);
> >>  DONE;
> >>})
>
> This pattern can work for current VLS modes so far since they are within 0~31, if we add more VLSmodes such as V32QImode, V64QImode,
> it can't work . I am ok with this, but I should remind you early.
 
Yeah, I Know the problem, my thought is we will have another set of
VLS patterns for those NUNITS >= 32, and require one clobber with GPR.
 
> Add tests with -march=rv64gcv_zvl256b to see whether your testcase can generate LMUL = mf2 vsetvli
>
> and -march=rv64gcv_zvl2048 make sure your testcase will not go into the VLS modes (2048 * 1 / 8 > 128)
 
I guess I should make a loop to test those combinations instead of
spearted file but with different options.
 
>
>
> For VSETVL part, I didn't see you define attribute sew/vlmul ...ratio for VLS modes.
>
> I wonder how these VLS modes emit correct VSETVL?
 
That's the magic I made here, I split the pattern after RA, but before
vsetvli, and convert all operands to VLA mode and use VLA pattern, so
that we don't need to modify any line of vsetvli stuff.