Hi!
On Mon, Feb 19, 2024 at 04:24:37PM +0530, Ajit Agarwal wrote:
> --- a/gcc/config.gcc
> +++ b/gcc/config.gcc
> @@ -518,7 +518,7 @@ or1k*-*-*)
> ;;
> powerpc*-*-*)
> cpu_type=rs6000
> - extra_objs="rs6000-string.o rs6000-p8swap.o rs6000-logue.o"
> + extra_objs="rs6000-string.o rs6000-p8swap.o rs6000-logue.o
> rs6000-vecload-fusion.o"
Line too long.
> + /* Pass to replace adjacent memory addresses lxv instruction with lxvp
> + instruction. */
> + INSERT_PASS_BEFORE (pass_early_remat, 1, pass_analyze_vecload);
That is not such a great name. Any pss name with "analyze" is not so
good -- the pass does much more than just "analyze" things!
> --- /dev/null
> +++ b/gcc/config/rs6000/rs6000-vecload-fusion.cc
> @@ -0,0 +1,701 @@
> +/* Subroutines used to replace lxv with lxvp
> + for TARGET_POWER10 and TARGET_VSX,
The pass filename is not good then, either.
> + Copyright (C) 2020-2023 Free Software Foundation, Inc.
What in here is from 2020?
Most things will be from 2024, too. First publication date is what
counts.
> + Contributed by Ajit Kumar Agarwal <aagar...@linux.ibm.com>.
We don't say such things in the files normally.
> +class rs6000_pair_fusion : public pair_fusion
> +{
> +public:
> + rs6000_pair_fusion (bb_info *bb) : pair_fusion (bb) {reg_ops = NULL;};
> + bool is_fpsimd_op_p (rtx reg_op, machine_mode mem_mode, bool load_p);
> + bool pair_mem_ok_policy (rtx first_mem, bool load_p, machine_mode mode)
> + {
> + return !(first_mem || load_p || mode);
> + }
It is much more natural to write this as
retuurn !first_mem && !load && !mode;
(_p is wrong, this is not a predicate, it is not a function at all!)
What is "!mode" for here? How can VOIDmode happen here? What does it
mean? This needs to be documented.
> + bool pair_check_register_operand (bool load_p, rtx reg_op,
> + machine_mode mem_mode)
> + {
> + if (load_p || reg_op || mem_mode)
> + return false;
> + else
> + return false;
> + }
The compiler will have warned for this. Please look at all compiler
(and other) warnings that you introduce.
> +rs6000_pair_fusion::is_fpsimd_op_p (rtx reg_op, machine_mode mem_mode, bool
> load_p)
> +{
> + return !((reg_op && mem_mode) || load_p);
> +}
For more complex logic, split it up into two or more conditional
returns.
> +// alias_walker that iterates over stores.
> +template<bool reverse, typename InsnPredicate>
> +class store_walker : public def_walker<reverse>
That is not a good comment. You should describe parameters and return
values and that kind of thing. That it walks over things is bloody
obvious from the name already :-)
> +extern insn_info *
> +find_trailing_add (insn_info *insns[2],
> + const insn_range_info &pair_range,
> + int initial_writeback,
> + rtx *writeback_effect,
> + def_info **add_def,
> + def_info *base_def,
> + poly_int64 initial_offset,
> + unsigned access_size);
That is way, way, way too many parameters.
So:
* Better names please.
* Better documentation, too, including documentations in the code.
Don't describe *what*, anyone can see that anyway, but describe *why*.
* This is way too much for one patch. Split this into many patches,
properly structured in a patch series. The design will need some
explanation, but none of the code should need that, ever!
Segher
