Re: [PATCH] RISC-V: Refactor the framework of RVV auto-vectorization

[Robin Dapp <rdapp.gcc@gmail.com>]

Hi Juzhe,

in general I find the revised structure quite logical and it is definitely
an improvement.  Some abstraction are still a bit leaky but we can always
refactor "on the fly".  Some comments on the general parts, skipping
over the later details.

>   bool m_has_dest_p;

Why does a store not have a destination (as commented below)?

>   /* It't true if the pattern uses all trues mask operand.  */
>   bool m_use_all_trues_mask_p;

m_all_unmasked_p or m_fully_unmasked_p?

>   /* It's true if the pattern uses undefined merge operand.  */
>   bool m_use_undef_merge_p;

Apart from the insn-centric name, couldn't we also decide this
based on the context later?  In the vector-builtins.cc we have
use_real_mask_p and use_real_merge_p that do this.

>   bool m_has_avl_p;

This means "has avl operand" I suppose?  From the caller's point
of view (and also the vsetvl pass) something like "needs avl" or so
would be more descriptive but undecided here.

>   bool m_vlmax_p;
>   bool m_has_tail_policy_p;
>   bool m_has_mask_policy_p;

Do we need to expose these in the constructor?  As far as I can
tell we can decide later whether the instruction has a policy
or not (as I did in my patch, depending on whether all inputs
are masks or so).

>   enum tail_policy m_tail_policy;
>   enum mask_policy m_mask_policy;

>   machine_mode m_dest_mode;
>   machine_mode m_mask_mode;

Having the mask mode be automatically deduced from the destination
is good, it was just obnoxious before to pass <VM>...

> Currently, we have "emit_vlmax_tany_many" and "emit_nonvlmax_tany_many".

I don't particularly like the names ;) Going back to vlmax and
nonvlmax I don't mind but do we really need to have the policies
encoded in the name now?  Especially since "many" is a word and
the default is ANY anyway.  Why not emit_vlmax_insn/emit_vlmax_op
for now and add the tu/mu later?
 
> #define RVV_BINOP_NUM 3 (number including the output)

Could make this into an "instruction type" rather than just a
number  (i.e. RVV_BINOP) and then set the number of operands
internally according to the type.  This would also make it clearer
in case we later want to set other options depending on the type.
 
> Then just use emit_vlmax_tany_many (...RVV_BINOP_NUM...)
> 
> So, if we support ternary operation in the future. It's quite simple:
> #define RVV_TERNOP_NUM 4 (number including the output)
> emit_vlmax_tany_many (...RVV_BINOP_NUM...)
> 
> "*_tany_many" means we are using tail any and mask any.
> 
> We will definitely need tail undisturbed or mask undisturbed when we support these patterns
> in middle-end. It's very simple to extend such helper base on current framework:
> 
> we can do that in the future like this:
> 
> void
> emit_nonvlmax_tu_mu (unsigned icode, int op_num, rtx *ops)
> {
>   machine_mode data_mode = GET_MODE (ops[0]);
>   machine_mode mask_mode = get_mask_mode (data_mode).require ();
>   /* The number = 11 is because we have maximum 11 operands for
>      RVV instruction patterns according to vector.md.  */

You can just drop the "The number = 11 is because" and say
"We have a maximum of 11 operands for...".

>   insn_expander<11> e (/*OP_NUM*/ op_num, /*HAS_DEST_P*/ true,
> 		       /*USE_ALL_TRUES_MASK_P*/ true,
> 		       /*USE_UNDEF_MERGE_P*/ true, /*HAS_AVL_P*/ true,
> 		       /*VLMAX_P*/ false,
> 		       /*HAS_TAIL_POLICY_P*/ true, /*HAS_MASK_POLICY_P*/ true,
> 		       /*TAIL_POLICY*/ TAIL_UNDISTURBED, /*MASK_POLICY*/ MASK_UNDISTURBED,
> 		       /*DEST_MODE*/ data_mode, /*MASK_MODE*/ mask_mode);
>   e.emit_insn ((enum insn_code) icode, ops);
> }

The eleven arguments seem a bit clunky here ;)  I would suggest
changing this again in the future bur for now let's just go ahead
with it in order to make progress.

> -  riscv_vector::emit_len_op (code_for_pred_mov (<MODE>mode), operands[0],
> -			     operands[1], operands[2], <VM>mode);
> +  riscv_vector::emit_nonvlmax_tany_many (code_for_pred_mov (<MODE>mode),
> +  					 RVV_UNOP_NUM, operands);
>    DONE;
>  })

The rtx operands[] array I like least of the changes in this patch.
It's essentially an untyped array whose meaning is dependent on context
containing source operands and the length that is sometimes empty and
sometimes not.  I can't think of something that wouldn't complicate things
though but before we at least had functions called _len that would take
a length (NULL or not) and _vlmax that wouldn't.  It's pretty easy to mess
up here on the caller's side.

That said, again, we can always refactor again and let's not let perfect
be the enemy of good here.  All in all it is not more complicated now than
it was before so we should go ahead IMHO.

Regards
 Robin