Hi Mike,
On Wed, May 10, 2023 at 11:38:55AM -0400, Michael Meissner wrote:
> This patch rewrites the gen_ld_cmpi_p10 function in genfusion.pl to be
> clearer.
That is not at all what I asked for, even if I would agree the code is
nicer to read now (I don't).
What I asked for, what is needed, is for your patches to be readable.
This is a prerequisite for them to be reviewable, which is a
prerequisite for them to be approvable. One way to do that is to split
out refactorings (which I asked for) and rewrites (which you did) to
earlier patches in the series. Pure refactoring are easy to review:
they change exactly nothing in what code is executed. Rewrites are much
harder to review. But even then we can hope you didn't slip up once in
a hundred lines of code, sure.
The later patches can then be much more readable because there isn't so
much noise mixed in.
> Assuming I can check this in, I will
> also commit to the active GCC branches after a burn-in period.
No, you will never do that. You always need approval for that. We have
these procedures for a reason. We do not want other things than what
was approved committed, doubly so if *nothing* was approved.
> * config/rs6000/genfusion.pl (mode_to_ldst_char): Delete.
This is a regression.
> +# Print the insns for load and compare with -1/0/1.
> +# Arguments:
> +# lmode -- Integer mode ("DI", "SI", "HI", or "QI").
> +# result -- "clobber", "GPR", or $lmode
> +# ccmode -- Sign vs. unsigned ("CC" or "CCUNS").
> +# mem_format -- Memory format ("d" or "ds").
> +# cmpl -- Suffix for compare ("l" or "")
> +# const_pred -- Predicate for constant (i.e. -1/0/1 or 0/1).
> +# extend -- "sign", "zero", or "none".
> +# echr -- Suffix for load ("a", "z", or "").
> +# load -- Load instruction (i.e. "ld", "lwa", "lwz", etc.)
> +# np -- enum non_prefixed_form for memory type
> +# constraint -- constraint to use
> +# mem_pred -- predicate for the memory operation
If you need a huge block comment for your sub argument, that is a
not-so-subtle hint that you need to refactor. Or if this was supposed
to be a refactoring, that something went terribly wrong :-(
Segher
