Thanks kito.
Address all comments and committed with V3:
https://gcc.gnu.org/pipermail/gcc-patches/2023-August/628423.html
juzhe.zh...@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 .
