Thanks Robin.

Sorry for not mentioned that it depends on another patch 
https://gcc.gnu.org/pipermail/gcc-patches/2023-May/619536.html, which is in the 
reviewing queue.

Yes, totally agree we can remove the comments for some parameters excepts the 
Boolean ones, as well as the term data related. I can file another PATCH to 
make it happen due to it is another thing besides magic number elimination.

Pan

-----Original Message-----
From: Robin Dapp <rdapp....@gmail.com> 
Sent: Friday, May 26, 2023 2:24 PM
To: Li, Pan2 <pan2...@intel.com>; gcc-patches@gcc.gnu.org
Cc: rdapp....@gmail.com; juzhe.zh...@rivai.ai; kito.ch...@sifive.com; Wang, 
Yanzhang <yanzhang.w...@intel.com>
Subject: Re: [PATCH] RISC-V: Eliminate the magic number in riscv-v.cc

Hi,

> This patch would like to remove the magic number in the riscv-v.cc, 
> and align the same value to one macro.

> diff --git a/gcc/config/riscv/riscv-v.cc b/gcc/config/riscv/riscv-v.cc 
> index 458020ce0a1..20b589bf51b 100644
> --- a/gcc/config/riscv/riscv-v.cc
> +++ b/gcc/config/riscv/riscv-v.cc
> @@ -351,13 +351,15 @@ emit_vlmax_insn (unsigned icode, int op_num, rtx 
> *ops, rtx vl)  {
>    machine_mode data_mode = GET_MODE (ops[0]);
>    machine_mode mask_mode = get_mask_mode (data_mode).require ();
> -  /* We have a maximum of 11 operands for RVV instruction patterns according 
> to
> -   * vector.md.  */
> -  insn_expander<11> e (/*OP_NUM*/ op_num, /*HAS_DEST_P*/ true,
> -                    /*FULLY_UNMASKED_P*/ true,
> -                    /*USE_REAL_MERGE_P*/ false, /*HAS_AVL_P*/ true,
> -                    /*VLMAX_P*/ true,
> -                    /*DEST_MODE*/ data_mode, /*MASK_MODE*/ mask_mode);
> +  insn_expander<RVV_INSN_OPERANDS_MAX> e (/*OP_NUM*/ op_num,
> +                                       /*HAS_DEST_P*/ true,
> +                                       /*FULLY_UNMASKED_P*/ true,
> +                                       /*USE_REAL_MERGE_P*/ false,
> +                                       /*HAS_AVL_P*/ true,
> +                                       /*VLMAX_P*/ true,
> +                                       /*DEST_MODE*/ data_mode,
> +                                       /*MASK_MODE*/ mask_mode);

I don't see where RVV_INSN_OPERANDS_MAX is defined.  Maybe you missed to 
include that hunk?

Apart from that maybe you could also remove the comments for dest_mode, 
mask_mode and op_num?  I think the general "custom" is to just add them for 
bool arguments and name non-bool arguments descriptively.  Here that could mean 
renaming data_mode to dest_mode where appropriate (usually data_mode is used to 
distinguish between data mode and comparison mode in conditionals, not in 
regular insns where everything is "data").

Regards
 Robin

Reply via email to