Re: [PATCH V2] RISC-V: Refactor Phase 3 (Demand fusion) of VSETVL PASS

[Kito Cheng <kito.cheng@sifive.com>]

>
>    -  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 .