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?