Re: Re: [PATCH] RISC-V: Support non-SLP unordered reduction

["juzhe.zhong@rivai.ai" <juzhe.zhong@rivai.ai>]

[-- Attachment #1: Type: text/plain, Size: 2705 bytes --]

Address comment.

V2 patch:
https://gcc.gnu.org/pipermail/gcc-patches/2023-July/624638.html 

I added:

+/* Change insn and Assert the change always happens.  */
+static void
+validate_change_or_fail (rtx object, rtx *loc, rtx new_rtx, bool in_group)
+{
+  bool change_p = validate_change (object, loc, new_rtx, in_group);
+  gcc_assert (change_p);
+}
as you suggested.

Could you take a look again?


juzhe.zhong@rivai.ai
 
From: Kito Cheng
Date: 2023-07-17 15:00
To: juzhe.zhong
CC: gcc-patches; kito.cheng; palmer; rdapp.gcc; jeffreyalaw
Subject: Re: [PATCH] RISC-V: Support non-SLP unordered reduction
> @@ -247,6 +248,7 @@ void emit_vlmax_cmp_mu_insn (unsigned, rtx *);
>  void emit_vlmax_masked_mu_insn (unsigned, int, rtx *);
>  void emit_scalar_move_insn (unsigned, rtx *);
>  void emit_nonvlmax_integer_move_insn (unsigned, rtx *, rtx);
> +//void emit_vlmax_reduction_insn (unsigned, rtx *);
 
Plz drop this.
 
 
> diff --git a/gcc/config/riscv/riscv-vsetvl.cc b/gcc/config/riscv/riscv-vsetvl.cc
> index 586dc8e5379..97a9dad8a77 100644
> --- a/gcc/config/riscv/riscv-vsetvl.cc
> +++ b/gcc/config/riscv/riscv-vsetvl.cc
> @@ -646,7 +646,8 @@ gen_vsetvl_pat (enum vsetvl_type insn_type, const vl_vtype_info &info, rtx vl)
>  }
>
>  static rtx
> -gen_vsetvl_pat (rtx_insn *rinsn, const vector_insn_info &info)
> +gen_vsetvl_pat (rtx_insn *rinsn, const vector_insn_info &info,
> +               rtx vl = NULL_RTX)
>  {
>    rtx new_pat;
>    vl_vtype_info new_info = info;
> @@ -657,7 +658,7 @@ gen_vsetvl_pat (rtx_insn *rinsn, const vector_insn_info &info)
>    if (vsetvl_insn_p (rinsn) || vlmax_avl_p (info.get_avl ()))
>      {
>        rtx dest = get_vl (rinsn);
 
rtx dest = vl ? vl : get_vl (rinsn);
 
> -      new_pat = gen_vsetvl_pat (VSETVL_NORMAL, new_info, dest);
> +      new_pat = gen_vsetvl_pat (VSETVL_NORMAL, new_info, vl ? vl : dest);
 
and keep dest here.
 
>      }
>    else if (INSN_CODE (rinsn) == CODE_FOR_vsetvl_vtype_change_only)
>      new_pat = gen_vsetvl_pat (VSETVL_VTYPE_CHANGE_ONLY, new_info, NULL_RTX);
 
Should we handle vl is non-null case in else-if and else case?
Add `assert (vl == NULL_RTX)` if not handle.
 
> @@ -818,7 +819,8 @@ change_insn (rtx_insn *rinsn, rtx new_pat)
>        print_rtl_single (dump_file, PATTERN (rinsn));
>      }
>
> -  validate_change (rinsn, &PATTERN (rinsn), new_pat, false);
> +  bool change_p = validate_change (rinsn, &PATTERN (rinsn), new_pat, false);
> +  gcc_assert (change_p);
 
I think we could create a wrapper for validate_change to make sure
that return true, and also use that wrapper for all other call sites?
 
e.g.
validate_change_or_fail?