On Fri, Mar 08, 2024 at 03:01:04AM +0530, Ajit Agarwal wrote:
>
> >> + 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.
Huh? This code won't be published before 2024 (and it does not derive
from older code), so the only valid date in the copyright message is
2024.
> >> + 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.
Hrm, apparently there is no -Wall -W warnign for this? But your code is
essentially
bool pair_check_register_operand (bool, rtx, machine_mode)
{
return false;
}
> >> +// 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.
It is required to decribe what a function is for, and all its arguments
and return values. If the aarch64 code doesn't, it should be fixed.
Not only reviewers need this, anyone trying to use the code needs it,
too.
> >> +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.
Don't copy not-so-good stuff unmodified? It is unreviewable, to start
with, but probably not very usable later either.
> Could you please elaborate on how you want me
> to structure the patches.
*You* should know the code already, so you surely can figure out a nice
way to present it, so that it takes me LESS work to review this than it
took you to write it?
Making a patch series reviewable is part of the development effort. It
is way less work if you start with this as the goal in mind. It is less
work than writing (and debugging etc.) an omnibus patch, in the first
place!
Your goal is to make a patch series that will be reviewed and then seen
to be great stuff. So it of course needs to *be* great stuff, but it
also needs to be presented in such a way that that is obvious.
Segher
