Thanks kito.
Address all comments and committed with V3:
https://gcc.gnu.org/pipermail/gcc-patches/2023-August/628423.html 



juzhe.zhong@rivai.ai
 
From: Kito Cheng
Date: 2023-08-25 01:01
To: Juzhe-Zhong
CC: gcc-patches; kito.cheng; jeffreyalaw; rdapp.gcc
Subject: Re: [PATCH V2] RISC-V: Refactor Phase 3 (Demand fusion) of VSETVL PASS
>
>    -  Phase 3 - Backward && forward demanded info propagation and fusion across
>       blocks.
>
 
Need update comment here.
 
>    -  Phase 6 - Propagate AVL between vsetvl instructions.
 
Need update comment here too.
 
> +/* Return true if the current VSETVL is dominated by preceding VSETVL.  */
> +static bool
> +vsetvl_dominated_by_p (const basic_block cfg_bb,
> +                      const vector_insn_info &vsetvl1,
> +                      const vector_insn_info &vsetvl2, bool fuse_p)
 
"VSETVL1 is dominated by preceding VSETVL2." ?
and what's the definition of dominated?
it seems like not in the traditional sense of "dominate"?
 
 
> vector_insn_info::merge (const vector_insn_info &merge_info,
> -                        enum merge_type type) const
> +                        enum merge_type type, int bb_index) const
 
I would suggest just split this into two funciton, local_merge and
global_merge, and remove merge_type,
generally I like generalized those function by arguments, but those
two are different enough after this change.
 
 
> +      /* Recompute the AVL source when bb_index*/
 
This sentence seems to be incomplete?
 
 
> +                 if (dest_block_info.probability > src_block_info.probability)
> +                   prob = dest_block_info.probability;
 
prob = std::max(dest_block_info.probability, src_block_info.probability);
 
> @@ -3720,6 +3138,8 @@ pass_vsetvl::compute_local_properties (void)
>    for (const bb_info *bb : crtl->ssa->bbs ())
>      {
>        unsigned int curr_bb_idx = bb->index ();
> +      if (curr_bb_idx == ENTRY_BLOCK || curr_bb_idx == EXIT_BLOCK)
> +       continue;
>        const auto local_dem
>         = m_vector_manager->vector_block_infos[curr_bb_idx].local_dem;
>        const auto reaching_out
 
This small change seems could be a small optimization for early exit
for this loop and could be a separated patch? if so plz send a
separated, and pre-aproved for that :)
 
 
 
> +             if (src_block_info.reaching_out.empty_p ())
> +               {
...
> +             else if (src_block_info.reaching_out.dirty_p ())
 
Could you add more comment to explain more for each condition?
 
> +       {
> +         rtx vl = NULL_RTX;
> +         if (!reaching_out.get_avl_source ())
> +           {
> +             gcc_assert (vsetvl_insn_p (reaching_out.get_insn ()->rtl ()));
> +             vl = get_vl (reaching_out.get_insn ()->rtl ());
> +           }
> +         else
> +           vl = reaching_out.get_avl_reg_rtx ();
> +         new_pat = gen_vsetvl_pat (VSETVL_NORMAL, reaching_out, vl);
> +       }
 
need more comment here too
 
> +      edge eg;
> +      edge_iterator eg_iterator;
> +      FOR_EACH_EDGE (eg, eg_iterator, cfg_bb->succs)
>         {
> -         fprintf (dump_file,
> -                  "\nInsert vsetvl insn %d at the end of <bb %d>:\n",
> -                  INSN_UID (new_insn), cfg_bb->index);
> -         print_rtl_single (dump_file, new_insn);
> +         /* We should not get an abnormal edge here.  */
> +         gcc_assert (!(eg->flags & EDGE_ABNORMAL));
> +         if (m_vector_manager->vsetvl_dominated_by_all_preds_p (cfg_bb,
> +                                                                reaching_out))
> +           continue;
> +
 
Also need more comments here .