Re: Re: [PATCH V2] RISC-V: Add RVV comparison autovectorization
Thanks a lot. Part of the comments has already been fixed in V4. But forget about V4 patch. Could you continue review V5 patch that I just send ? https://gcc.gnu.org/pipermail/gcc-patches/2023-May/619366.html with all comments from you have been fixed. Thanks. juzhe.zh...@rivai.ai From: Kito Cheng Date: 2023-05-24 11:20 To: juzhe.zhong CC: gcc-patches; kito.cheng; palmer; palmer; jeffreyalaw; rdapp.gcc; Richard Sandiford Subject: Re: [PATCH V2] RISC-V: Add RVV comparison autovectorization > +void > +expand_vec_cmp (rtx target, rtx_code code, rtx mask, rtx maskoff, rtx op0, > + rtx op1) > ... > + rtx cmp = gen_rtx_fmt_ee (code, mask_mode, op0, op1); > + rtx ops[RVV_CMP_OP + 2] = {target, mask, maskoff, cmp, op0, op1}; > + emit_vlmax_cmp_insn (icode, RVV_CMP_OP + 2, ops); It's too magic. > +/* This function emits cmp instruction. */ > +void > +emit_vlmax_cmp_insn (unsigned icode, int op_num, rtx *ops) > +{ > + machine_mode mode = GET_MODE (ops[0]); > + bool fully_unmasked_p = op_num == RVV_CMP_OP ? true : false; > + bool use_real_merge_p = op_num == RVV_CMP_OP ? false : true; Don't do that, plz separate break this function into two. > + /* 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*/ fully_unmasked_p, > + /*USE_REAL_MERGE_P*/ use_real_merge_p, > + /*HAS_AVL_P*/ true, > + /*VLMAX_P*/ true, > + /*DEST_MODE*/ mode, /*MASK_MODE*/ mode); > + e.set_policy (op_num == RVV_CMP_OP ? MASK_UNDISTURBED : MASK_ANY); > + e.emit_insn ((enum insn_code) icode, ops); > +} > + > /* Expand series const vector. */ > > void > +void > +expand_vec_cmp (rtx target, rtx_code code, rtx op0, rtx op1) > +{ > + machine_mode mask_mode = GET_MODE (target); > + machine_mode data_mode = GET_MODE (op0); > + insn_code icode = get_cmp_insn_code (code, data_mode); > + > + if (code == LTGT) > +{ > + rtx gt = gen_reg_rtx (mask_mode); > + rtx lt = gen_reg_rtx (mask_mode); > + expand_vec_cmp (gt, GT, op0, op1); > + expand_vec_cmp (lt, LT, op0, op1); > + icode = code_for_pred (IOR, mask_mode); > + rtx ops[3] = {target, gt, lt}; rtx ops[] = {target, gt, lt}; > + emit_vlmax_insn (icode, riscv_vector::RVV_BINOP, ops); > + return; > +} > + > + rtx cmp = gen_rtx_fmt_ee (code, mask_mode, op0, op1); > + rtx ops[RVV_CMP_OP] = {target, cmp, op0, op1}; rtx ops[] = {target, cmp, op0, op1}; > + emit_vlmax_cmp_insn (icode, RVV_CMP_OP, ops); > +} > + > + /* There is native support for the inverse comparison. */ > + code = reverse_condition_maybe_unordered (code); > + if (code == ORDERED) > +emit_move_insn (target, eq0); > + else > +expand_vec_cmp (eq0, code, eq0, eq0, op0, op1); > + > + if (can_invert_p) > +{ > + emit_move_insn (target, eq0); > + return true; > +} > + insn_code icode = code_for_pred_not (mask_mode); > + rtx ops[RVV_UNOP] = {target, eq0}; > + emit_vlmax_insn (icode, RVV_UNOP, ops); rtx ops[] = {target, eq0};
Re: [PATCH V2] RISC-V: Add RVV comparison autovectorization
> +void > +expand_vec_cmp (rtx target, rtx_code code, rtx mask, rtx maskoff, rtx op0, > + rtx op1) > ... > + rtx cmp = gen_rtx_fmt_ee (code, mask_mode, op0, op1); > + rtx ops[RVV_CMP_OP + 2] = {target, mask, maskoff, cmp, op0, op1}; > + emit_vlmax_cmp_insn (icode, RVV_CMP_OP + 2, ops); It's too magic. > +/* This function emits cmp instruction. */ > +void > +emit_vlmax_cmp_insn (unsigned icode, int op_num, rtx *ops) > +{ > + machine_mode mode = GET_MODE (ops[0]); > + bool fully_unmasked_p = op_num == RVV_CMP_OP ? true : false; > + bool use_real_merge_p = op_num == RVV_CMP_OP ? false : true; Don't do that, plz separate break this function into two. > + /* 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*/ fully_unmasked_p, > + /*USE_REAL_MERGE_P*/ use_real_merge_p, > + /*HAS_AVL_P*/ true, > + /*VLMAX_P*/ true, > + /*DEST_MODE*/ mode, /*MASK_MODE*/ mode); > + e.set_policy (op_num == RVV_CMP_OP ? MASK_UNDISTURBED : MASK_ANY); > + e.emit_insn ((enum insn_code) icode, ops); > +} > + > /* Expand series const vector. */ > > void > +void > +expand_vec_cmp (rtx target, rtx_code code, rtx op0, rtx op1) > +{ > + machine_mode mask_mode = GET_MODE (target); > + machine_mode data_mode = GET_MODE (op0); > + insn_code icode = get_cmp_insn_code (code, data_mode); > + > + if (code == LTGT) > +{ > + rtx gt = gen_reg_rtx (mask_mode); > + rtx lt = gen_reg_rtx (mask_mode); > + expand_vec_cmp (gt, GT, op0, op1); > + expand_vec_cmp (lt, LT, op0, op1); > + icode = code_for_pred (IOR, mask_mode); > + rtx ops[3] = {target, gt, lt}; rtx ops[] = {target, gt, lt}; > + emit_vlmax_insn (icode, riscv_vector::RVV_BINOP, ops); > + return; > +} > + > + rtx cmp = gen_rtx_fmt_ee (code, mask_mode, op0, op1); > + rtx ops[RVV_CMP_OP] = {target, cmp, op0, op1}; rtx ops[] = {target, cmp, op0, op1}; > + emit_vlmax_cmp_insn (icode, RVV_CMP_OP, ops); > +} > + > + /* There is native support for the inverse comparison. */ > + code = reverse_condition_maybe_unordered (code); > + if (code == ORDERED) > +emit_move_insn (target, eq0); > + else > +expand_vec_cmp (eq0, code, eq0, eq0, op0, op1); > + > + if (can_invert_p) > +{ > + emit_move_insn (target, eq0); > + return true; > +} > + insn_code icode = code_for_pred_not (mask_mode); > + rtx ops[RVV_UNOP] = {target, eq0}; > + emit_vlmax_insn (icode, RVV_UNOP, ops); rtx ops[] = {target, eq0};
Re: Re: [PATCH V2] RISC-V: Add RVV comparison autovectorization
Ok. Let's wait for Kito's more comments. Thanks. juzhe.zh...@rivai.ai From: Robin Dapp Date: 2023-05-24 05:07 To: 钟居哲; gcc-patches CC: rdapp.gcc; kito.cheng; kito.cheng; palmer; palmer; Jeff Law; richard.sandiford Subject: Re: [PATCH V2] RISC-V: Add RVV comparison autovectorization >>> Don't you want to use your shiny new operand passing style here as >>> with the other expanders? > H, I do this just following ARM code style. > You can see I do pass rtx[] for expand_vcond and pass rtx,rtx,rtx for > expand_vec_cmp. > Well, I just follow ARM SVE implementation (You can check aarch64-sve.md, we > are the same) :) > If don't like it, could give me more information then I change it for you. It doesn't matter that much in the end. I just wondered that we just introduced a new style of passing operands to the insn_expander and then immediately not use it in the first follow up :) Nit: + e.set_policy (op_num == RVV_CMP_OP ? MASK_UNDISTURBED : MASK_ANY); This looks weird in an emit__cmp_insn. Without a comment it's unclear why anything else but a CMP_OP would ever be used here. The double meaning of the enum (that I wanted to be an instruction type rather than a "number of operands") doesn't help. But well, fixable in the future. We just need to make sure not to accumulate too many of these warts. From the expander side V3 looks clean now. The integer parts look OK to me but I haven't checked the FP side at all. Regards Robin
Re: [PATCH V2] RISC-V: Add RVV comparison autovectorization
>>> Don't you want to use your shiny new operand passing style here as >>> with the other expanders? > H, I do this just following ARM code style. > You can see I do pass rtx[] for expand_vcond and pass rtx,rtx,rtx for > expand_vec_cmp. > Well, I just follow ARM SVE implementation (You can check aarch64-sve.md, we > are the same) :) > If don't like it, could give me more information then I change it for you. It doesn't matter that much in the end. I just wondered that we just introduced a new style of passing operands to the insn_expander and then immediately not use it in the first follow up :) Nit: + e.set_policy (op_num == RVV_CMP_OP ? MASK_UNDISTURBED : MASK_ANY); This looks weird in an emit__cmp_insn. Without a comment it's unclear why anything else but a CMP_OP would ever be used here. The double meaning of the enum (that I wanted to be an instruction type rather than a "number of operands") doesn't help. But well, fixable in the future. We just need to make sure not to accumulate too many of these warts. >From the expander side V3 looks clean now. The integer parts look OK to me but I haven't checked the FP side at all. Regards Robin
Re: Re: [PATCH V2] RISC-V: Add RVV comparison autovectorization
Hi, Robin. >> Don't you want to use your shiny new operand passing style here as >> with the other expanders? H, I do this just following ARM code style. You can see I do pass rtx[] for expand_vcond and pass rtx,rtx,rtx for expand_vec_cmp. Well, I just follow ARM SVE implementation (You can check aarch64-sve.md, we are the same) :) If don't like it, could give me more information then I change it for you. >> I don't think we need the same comment in each of these. Same for >> /*DEST_MODE*/ and /*MASK_MODE*/ which would be redundant if data_mode >> were called dest_mode. Ok >> Swap lt and gt here for consistency's sake. Ok. I have fixed as you suggested. Would you mind review V3 patch: https://gcc.gnu.org/pipermail/gcc-patches/2023-May/619324.html Thanks. juzhe.zh...@rivai.ai From: Robin Dapp Date: 2023-05-23 22:12 To: juzhe.zhong; gcc-patches CC: rdapp.gcc; kito.cheng; kito.cheng; palmer; palmer; jeffreyalaw; Richard Sandiford Subject: Re: [PATCH V2] RISC-V: Add RVV comparison autovectorization > +(define_expand "vec_cmp" > + [(set (match_operand: 0 "register_operand") > + (match_operator: 1 "comparison_operator" > + [(match_operand:VI 2 "register_operand") > +(match_operand:VI 3 "register_operand")]))] > + "TARGET_VECTOR" > + { > +riscv_vector::expand_vec_cmp (operands[0], GET_CODE (operands[1]), > + operands[2], operands[3]); > +DONE; > + } > +) > + > +(define_expand "vec_cmpu" > + [(set (match_operand: 0 "register_operand") > + (match_operator: 1 "comparison_operator" > + [(match_operand:VI 2 "register_operand") > +(match_operand:VI 3 "register_operand")]))] > + "TARGET_VECTOR" > + { > +riscv_vector::expand_vec_cmp (operands[0], GET_CODE (operands[1]), > + operands[2], operands[3]); > +DONE; > + } > +) > + > +(define_expand "vec_cmp" > + [(set (match_operand: 0 "register_operand") > + (match_operator: 1 "comparison_operator" > + [(match_operand:VF 2 "register_operand") > +(match_operand:VF 3 "register_operand")]))] > + "TARGET_VECTOR" > + { > +riscv_vector::expand_vec_cmp_float (operands[0], GET_CODE (operands[1]), > + operands[2], operands[3], false); > +DONE; > + } > +) Don't you want to use your shiny new operand passing style here as with the other expanders? > + /* 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*/ false, > +/*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.emit_insn ((enum insn_code) icode, ops); > +} I don't think we need the same comment in each of these. Same for /*DEST_MODE*/ and /*MASK_MODE*/ which would be redundant if data_mode were called dest_mode. > +/* Expand an RVV comparison. */ > + > +void > +expand_vec_cmp (rtx target, rtx_code code, rtx op0, rtx op1) > +{ > + machine_mode mask_mode = GET_MODE (target); > + machine_mode data_mode = GET_MODE (op0); > + insn_code icode = get_cmp_insn_code (code, data_mode); > + > + if (code == LTGT) > +{ > + rtx gt = gen_reg_rtx (mask_mode); > + rtx lt = gen_reg_rtx (mask_mode); > + expand_vec_cmp (gt, GT, op0, op1); > + expand_vec_cmp (lt, LT, op0, op1); > + icode = code_for_pred (IOR, mask_mode); > + rtx ops[3] = {target, gt, lt}; > + emit_vlmax_insn (icode, riscv_vector::RVV_BINOP, ops); > + return; > +} Swap lt and gt here for consistency's sake. Regards Robin ail-LinkSize:2273655 QQMail-LineLen:76 QQMail-BreakType:1 QQMail-Key:cbdff912c7f03cb40444ad0dccf1f041 QQMail-MD5:6754fd07de754a129fff82b243962497 QQMail-LinkEnd --=_Part_2195_841924464.1657529212753--0eWxlPSJjb2xvcjojMDAwMDAwIj48Zm9udCB5YWhlaT0i Ij48c3Ryb25nPlRlbDo8L3N0cm9uZz4mbmJzcDs4Ni0yOC02ODM3MzE2NiA2ODM3MzE4OCZuYnNw OzxiciAvPgo8c3Ryb25nPkZheDo8L3N0cm9uZz4mbmJzcDs4Ni0yOC02ODM3MzE2Ni04MDQmbmJz cDs8YnIgLz4KPHN0cm9uZz5BZGQ6PC9zdHJvbmc+NzE4LE5vLjEwLDEgTm9ydGgsMiBSaW5nLENo ZW5nZHUsQ2hpbmEsPC9mb250PjwvZm9udD48L2ZvbnQ+PGJyIG1pY3Jvc29mdD0iIiBzdHlsZT0i Y29sb3I6IzAwMDAwMCIgeWFoZWk9IiIgLz4KPGZvbnQgbWljcm9zb2Z0PSIiPjxmb250IHN0eWxl PSJjb2xvcjojMDAwMDAwIj48Zm9udCB5YWhlaT0iIj48c3Ryb25nPlBvc3RhbCBjb2RlOjwvc3Ry b25nPjYxMDAzMTwvZm9udD48L2ZvbnQ+PC9mb250PjxiciBtaWNyb3NvZnQ9IiIgc3R5bGU9ImNv bG9yOiMwMDAwMDAiIHlhaGVpPSIiIC8+CiZuYnNwOzxpbWcgYWx0PSIiIHNyYz0iL2VudHNvZnQv RXRBY3Rpb24uZW50Y3JtP21ldGhvZD10ZSZtYWlsSUQ9ODgwN
Re: [PATCH V2] RISC-V: Add RVV comparison autovectorization
> +(define_expand "vec_cmp" > + [(set (match_operand: 0 "register_operand") > + (match_operator: 1 "comparison_operator" > + [(match_operand:VI 2 "register_operand") > +(match_operand:VI 3 "register_operand")]))] > + "TARGET_VECTOR" > + { > +riscv_vector::expand_vec_cmp (operands[0], GET_CODE (operands[1]), > + operands[2], operands[3]); > +DONE; > + } > +) > + > +(define_expand "vec_cmpu" > + [(set (match_operand: 0 "register_operand") > + (match_operator: 1 "comparison_operator" > + [(match_operand:VI 2 "register_operand") > +(match_operand:VI 3 "register_operand")]))] > + "TARGET_VECTOR" > + { > +riscv_vector::expand_vec_cmp (operands[0], GET_CODE (operands[1]), > + operands[2], operands[3]); > +DONE; > + } > +) > + > +(define_expand "vec_cmp" > + [(set (match_operand: 0 "register_operand") > + (match_operator: 1 "comparison_operator" > + [(match_operand:VF 2 "register_operand") > +(match_operand:VF 3 "register_operand")]))] > + "TARGET_VECTOR" > + { > +riscv_vector::expand_vec_cmp_float (operands[0], GET_CODE (operands[1]), > + operands[2], operands[3], false); > +DONE; > + } > +) Don't you want to use your shiny new operand passing style here as with the other expanders? > + /* 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*/ false, > +/*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.emit_insn ((enum insn_code) icode, ops); > +} I don't think we need the same comment in each of these. Same for /*DEST_MODE*/ and /*MASK_MODE*/ which would be redundant if data_mode were called dest_mode. > +/* Expand an RVV comparison. */ > + > +void > +expand_vec_cmp (rtx target, rtx_code code, rtx op0, rtx op1) > +{ > + machine_mode mask_mode = GET_MODE (target); > + machine_mode data_mode = GET_MODE (op0); > + insn_code icode = get_cmp_insn_code (code, data_mode); > + > + if (code == LTGT) > +{ > + rtx gt = gen_reg_rtx (mask_mode); > + rtx lt = gen_reg_rtx (mask_mode); > + expand_vec_cmp (gt, GT, op0, op1); > + expand_vec_cmp (lt, LT, op0, op1); > + icode = code_for_pred (IOR, mask_mode); > + rtx ops[3] = {target, gt, lt}; > + emit_vlmax_insn (icode, riscv_vector::RVV_BINOP, ops); > + return; > +} Swap lt and gt here for consistency's sake. Regards Robin
[PATCH V2] RISC-V: Add RVV comparison autovectorization
From: Juzhe-Zhong This patch enable RVV auto-vectorization including floating-point unorder and order comparison. The testcases are leveraged from Richard. So include Richard as co-author. Co-Authored-By: Richard Sandiford gcc/ChangeLog: * config/riscv/autovec.md (@vcond_mask_): New pattern. (vec_cmp): Ditto. (vec_cmpu): Ditto. (vcond): Ditto. (vcondu): Ditto. * config/riscv/riscv-protos.h (enum insn_type): Add new enum. (emit_vlmax_merge_insn): New function. (emit_vlmax_cmp_insn): Ditto. (expand_vec_cmp): Ditto. (expand_vec_cmp_float):Ditto. (expand_vcond):Ditto. * config/riscv/riscv-v.cc (emit_vlmax_merge_insn): Ditto. (emit_vlmax_cmp_insn): Ditto. (get_cmp_insn_code): Ditto. (expand_vec_cmp): Ditto. (expand_vec_cmp_float): Ditto. (expand_vcond): Ditto. gcc/testsuite/ChangeLog: * gcc.target/riscv/rvv/rvv.exp: Add RVV comparison testcases. * gcc.target/riscv/rvv/autovec/cmp/vcond-1.c: New test. * gcc.target/riscv/rvv/autovec/cmp/vcond-2.c: New test. * gcc.target/riscv/rvv/autovec/cmp/vcond-3.c: New test. * gcc.target/riscv/rvv/autovec/cmp/vcond_run-1.c: New test. * gcc.target/riscv/rvv/autovec/cmp/vcond_run-2.c: New test. * gcc.target/riscv/rvv/autovec/cmp/vcond_run-3.c: New test. --- gcc/config/riscv/autovec.md | 112 gcc/config/riscv/riscv-protos.h | 7 + gcc/config/riscv/riscv-v.cc | 258 +- .../riscv/rvv/autovec/cmp/vcond-1.c | 157 +++ .../riscv/rvv/autovec/cmp/vcond-2.c | 75 + .../riscv/rvv/autovec/cmp/vcond-3.c | 13 + .../riscv/rvv/autovec/cmp/vcond_run-1.c | 49 .../riscv/rvv/autovec/cmp/vcond_run-2.c | 76 ++ .../riscv/rvv/autovec/cmp/vcond_run-3.c | 6 + gcc/testsuite/gcc.target/riscv/rvv/rvv.exp| 2 + 10 files changed, 754 insertions(+), 1 deletion(-) create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/autovec/cmp/vcond-1.c create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/autovec/cmp/vcond-2.c create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/autovec/cmp/vcond-3.c create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/autovec/cmp/vcond_run-1.c create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/autovec/cmp/vcond_run-2.c create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/autovec/cmp/vcond_run-3.c diff --git a/gcc/config/riscv/autovec.md b/gcc/config/riscv/autovec.md index 04b4459222a..e0258e8b798 100644 --- a/gcc/config/riscv/autovec.md +++ b/gcc/config/riscv/autovec.md @@ -162,3 +162,115 @@ riscv_vector::RVV_BINOP, operands); DONE; }) + +;; = +;; == Comparisons and selects +;; = + +;; - +;; [INT,FP] Select based on masks +;; - +;; Includes merging patterns for: +;; - vmerge.vv +;; - vmerge.vx +;; - vfmerge.vf +;; - + +(define_expand "@vcond_mask_" + [(match_operand:V 0 "register_operand") + (match_operand: 3 "register_operand") + (match_operand:V 1 "nonmemory_operand") + (match_operand:V 2 "register_operand")] + "TARGET_VECTOR" + { +/* The order of vcond_mask is opposite to pred_merge. */ +std::swap (operands[1], operands[2]); +riscv_vector::emit_vlmax_merge_insn (code_for_pred_merge (mode), + riscv_vector::RVV_MERGE_OP, operands); +DONE; + } +) + +;; - +;; [INT,FP] Comparisons +;; - +;; Includes: +;; - vms. +;; - + +(define_expand "vec_cmp" + [(set (match_operand: 0 "register_operand") + (match_operator: 1 "comparison_operator" + [(match_operand:VI 2 "register_operand") + (match_operand:VI 3 "register_operand")]))] + "TARGET_VECTOR" + { +riscv_vector::expand_vec_cmp (operands[0], GET_CODE (operands[1]), + operands[2], operands[3]); +DONE; + } +) + +(define_expand "vec_cmpu" + [(set (match_operand: 0 "register_operand") + (match_operator: 1 "comparison_operator" + [(match_operand:VI 2 "register_operand") + (match_operand:VI 3 "register_operand")]))] + "TARGET_VECTOR" + { +riscv_vector::expand_vec_cmp (operands[0], GET_CODE (operands[1]), + operands[2], operands[3]); +DONE; + } +) + +(define_expand "vec_cmp" + [(set