RISC-V: Eliminate the magic number in riscv-v.cc
Checks
Commit Message
From: Pan Li <pan2.li@intel.com>
This patch would like to remove the magic number in the riscv-v.cc, and
align the same value to one macro.
Signed-off-by: Pan Li <pan2.li@intel.com>
gcc/ChangeLog:
* config/riscv/riscv-v.cc (emit_vlmax_insn): Eliminate the
magic number.
(emit_nonvlmax_insn): Ditto.
(emit_vlmax_merge_insn): Ditto.
(emit_vlmax_cmp_insn): Ditto.
(emit_vlmax_cmp_mu_insn): Ditto.
(expand_vec_series): Ditto.
Signed-off-by: Pan Li <pan2.li@intel.com>
---
gcc/config/riscv/riscv-v.cc | 77 ++++++++++++++++++++++---------------
1 file changed, 46 insertions(+), 31 deletions(-)
Comments
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
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.gcc@gmail.com>
Sent: Friday, May 26, 2023 2:24 PM
To: Li, Pan2 <pan2.li@intel.com>; gcc-patches@gcc.gnu.org
Cc: rdapp.gcc@gmail.com; juzhe.zhong@rivai.ai; kito.cheng@sifive.com; Wang, Yanzhang <yanzhang.wang@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
LGTM
On Fri, May 26, 2023 at 2:32 PM Li, Pan2 via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> 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.gcc@gmail.com>
> Sent: Friday, May 26, 2023 2:24 PM
> To: Li, Pan2 <pan2.li@intel.com>; gcc-patches@gcc.gnu.org
> Cc: rdapp.gcc@gmail.com; juzhe.zhong@rivai.ai; kito.cheng@sifive.com; Wang, Yanzhang <yanzhang.wang@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
Thanks Kito, will commit this after the vec_init repeated sequence patch.
Pan
-----Original Message-----
From: Kito Cheng <kito.cheng@gmail.com>
Sent: Monday, May 29, 2023 10:20 AM
To: Li, Pan2 <pan2.li@intel.com>
Cc: Robin Dapp <rdapp.gcc@gmail.com>; gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@sifive.com; Wang, Yanzhang <yanzhang.wang@intel.com>
Subject: Re: [PATCH] RISC-V: Eliminate the magic number in riscv-v.cc
LGTM
On Fri, May 26, 2023 at 2:32 PM Li, Pan2 via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>
> 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.gcc@gmail.com>
> Sent: Friday, May 26, 2023 2:24 PM
> To: Li, Pan2 <pan2.li@intel.com>; gcc-patches@gcc.gnu.org
> Cc: rdapp.gcc@gmail.com; juzhe.zhong@rivai.ai; kito.cheng@sifive.com;
> Wang, Yanzhang <yanzhang.wang@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
Committed, thanks Kito.
Pan
-----Original Message-----
From: Li, Pan2
Sent: Monday, May 29, 2023 1:38 PM
To: Kito Cheng <kito.cheng@gmail.com>
Cc: Robin Dapp <rdapp.gcc@gmail.com>; gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@sifive.com; Wang, Yanzhang <yanzhang.wang@intel.com>
Subject: RE: [PATCH] RISC-V: Eliminate the magic number in riscv-v.cc
Thanks Kito, will commit this after the vec_init repeated sequence patch.
Pan
-----Original Message-----
From: Kito Cheng <kito.cheng@gmail.com>
Sent: Monday, May 29, 2023 10:20 AM
To: Li, Pan2 <pan2.li@intel.com>
Cc: Robin Dapp <rdapp.gcc@gmail.com>; gcc-patches@gcc.gnu.org; juzhe.zhong@rivai.ai; kito.cheng@sifive.com; Wang, Yanzhang <yanzhang.wang@intel.com>
Subject: Re: [PATCH] RISC-V: Eliminate the magic number in riscv-v.cc
LGTM
On Fri, May 26, 2023 at 2:32 PM Li, Pan2 via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
>
> 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.gcc@gmail.com>
> Sent: Friday, May 26, 2023 2:24 PM
> To: Li, Pan2 <pan2.li@intel.com>; gcc-patches@gcc.gnu.org
> Cc: rdapp.gcc@gmail.com; juzhe.zhong@rivai.ai; kito.cheng@sifive.com;
> Wang, Yanzhang <yanzhang.wang@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
@@ -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);
+
e.set_policy (TAIL_ANY);
e.set_policy (MASK_ANY);
/* According to LRA mov pattern in vector.md, we have a clobber operand
@@ -373,13 +375,15 @@ emit_nonvlmax_insn (unsigned icode, int op_num, rtx *ops, rtx avl)
{
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*/ false,
- /*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*/ false,
+ /*DEST_MODE*/ data_mode,
+ /*MASK_MODE*/ mask_mode);
+
e.set_policy (TAIL_ANY);
e.set_policy (MASK_ANY);
e.set_vl (avl);
@@ -392,10 +396,15 @@ emit_vlmax_merge_insn (unsigned icode, int op_num, rtx *ops)
{
machine_mode dest_mode = GET_MODE (ops[0]);
machine_mode mask_mode = get_mask_mode (dest_mode).require ();
- insn_expander<11> e (/*OP_NUM*/ op_num, /*HAS_DEST_P*/ true,
- /*FULLY_UNMASKED_P*/ false,
- /*USE_REAL_MERGE_P*/ false, /*HAS_AVL_P*/ true,
- /*VLMAX_P*/ true, dest_mode, mask_mode);
+ insn_expander<RVV_INSN_OPERANDS_MAX> e (/*OP_NUM*/ op_num,
+ /*HAS_DEST_P*/ true,
+ /*FULLY_UNMASKED_P*/ false,
+ /*USE_REAL_MERGE_P*/ false,
+ /*HAS_AVL_P*/ true,
+ /*VLMAX_P*/ true,
+ /*DEST_MODE*/ dest_mode,
+ /*MASK_MODE*/ mask_mode);
+
e.set_policy (TAIL_ANY);
e.emit_insn ((enum insn_code) icode, ops);
}
@@ -405,12 +414,15 @@ void
emit_vlmax_cmp_insn (unsigned icode, rtx *ops)
{
machine_mode mode = GET_MODE (ops[0]);
- insn_expander<11> e (/*OP_NUM*/ RVV_CMP_OP, /*HAS_DEST_P*/ true,
- /*FULLY_UNMASKED_P*/ true,
- /*USE_REAL_MERGE_P*/ false,
- /*HAS_AVL_P*/ true,
- /*VLMAX_P*/ true,
- /*DEST_MODE*/ mode, /*MASK_MODE*/ mode);
+ insn_expander<RVV_INSN_OPERANDS_MAX> e (/*OP_NUM*/ RVV_CMP_OP,
+ /*HAS_DEST_P*/ true,
+ /*FULLY_UNMASKED_P*/ true,
+ /*USE_REAL_MERGE_P*/ false,
+ /*HAS_AVL_P*/ true,
+ /*VLMAX_P*/ true,
+ /*DEST_MODE*/ mode,
+ /*MASK_MODE*/ mode);
+
e.set_policy (MASK_ANY);
e.emit_insn ((enum insn_code) icode, ops);
}
@@ -420,12 +432,15 @@ void
emit_vlmax_cmp_mu_insn (unsigned icode, rtx *ops)
{
machine_mode mode = GET_MODE (ops[0]);
- insn_expander<11> e (/*OP_NUM*/ RVV_CMP_MU_OP, /*HAS_DEST_P*/ true,
- /*FULLY_UNMASKED_P*/ false,
- /*USE_REAL_MERGE_P*/ true,
- /*HAS_AVL_P*/ true,
- /*VLMAX_P*/ true,
- /*DEST_MODE*/ mode, /*MASK_MODE*/ mode);
+ insn_expander<RVV_INSN_OPERANDS_MAX> e (/*OP_NUM*/ RVV_CMP_MU_OP,
+ /*HAS_DEST_P*/ true,
+ /*FULLY_UNMASKED_P*/ false,
+ /*USE_REAL_MERGE_P*/ true,
+ /*HAS_AVL_P*/ true,
+ /*VLMAX_P*/ true,
+ /*DEST_MODE*/ mode,
+ /*MASK_MODE*/ mode);
+
e.set_policy (MASK_UNDISTURBED);
e.emit_insn ((enum insn_code) icode, ops);
}
@@ -443,7 +458,7 @@ expand_vec_series (rtx dest, rtx base, rtx step)
/* Step 1: Generate I = { 0, 1, 2, ... } by vid.v. */
rtx vid = gen_reg_rtx (mode);
- rtx op[1] = {vid};
+ rtx op[] = {vid};
emit_vlmax_insn (code_for_pred_series (mode), RVV_MISC_OP, op);
/* Step 2: Generate I * STEP.