Hello Segher:
On 01/03/24 3:02 am, Segher Boessenkool wrote:
> 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.
I will incorporate this change.
>
>> + /* 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!
>
I will change that and incorporate that.
>> --- /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.
>
I will change and incorporate it.
>> + 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.
Please let me know the second publication date.
>
>> + Contributed by Ajit Kumar Agarwal <aagar...@linux.ibm.com>.
>
> We don't say such things in the files normally.
>
Yes I will remove it.
>> +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!)
>
Surely I will do that.
> What is "!mode" for here? How can VOIDmode happen here? What does it
> mean? This needs to be documented.
>
Yes I will document that.
>> + 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.
>
As far as my understanding I didn't see any extra warnings,
but I will surely cross check and solve that.
>> +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.
>
Surely I will do that.
>> +// 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 :-)
>
This part of code is taken from aarch64 load store fusion
pass. I have made the aarch64-ldp-fusion.cc into target independent code and
target dependent code. Target independent code is shared
across all the architecture, In this case its rs6000 and aarch64.
Target dependent code is implemented through pure virtual functions.
While doing this, I have not changed target independent code
taken as it is from aarch64-ldp-fusion.cc
This is how they have added comments.
Target dependent code is based in rs6000-vecload-fusion.cc and
aarch64-ldp-fusion.cc.
Target independent code is populated in 3 files.
gcc/pair-fusion-base.h
This file has declaration of pair_fusion base class and
other classes declarations along with prototype of common
fusions in gcc/pair-fusion-common.cc
gcc/pair-fusion-common.cc
Here we have common function that is shared across all
architectures that are helper functions that are used
inside pair_fusion class member functions.
gcc/pair-fusion.cc
These are implementation of member function of pair_fusion
class.
Architecture dependent files are
rs6000-vecload-fusion-pass/aarch64-ldp-fusion.cc. This has implementation of
derived classes from pair_fusion classes
and target specific code added to it.
>> +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.
>
This code I have taken from aarch64-ldp-fusion.cc.
I have not changed anything here.
> 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.
Sure will do that.
Split this into many patches,
> properly structured in a patch series.
Could you please elaborate on how you want me
to structure the patches.
one way would be to split into target independent
and target independent code, but those will not
be independent patches.
Please let me know.
The design will need some
> explanation, but none of the code should need that, ever!
>>
Thanks & Regards
Ajit
> Segher
