Re: [PATCH V3 0/7] ira/lra: Support subreg coalesce

[Vladimir Makarov <vmakarov@redhat.com>]

On 11/12/23 07:08, Lehua Ding wrote:
> V3 Changes:
>    1. fix three ICE.
>    2. rebase
>
> Hi,
>
> These patchs try to support subreg coalesce feature in
> register allocation passes (ira and lra).
>
I've started review of v3 patches and here is my initial general 
criticism of your patches:

   * Absence of comments for some functions, e.g. for `HARD_REG_SET 
operator>> (unsigned int shift_amount) const`.

   * Adding significant functionality to existing functions is not 
reflected in the function comment, e.g. in ira_set_allocno_class.

   * A lot of typos, e.g. `pesudo` or `reprensent`.  I think you need to 
check spelling of you comments (I myself do spell checking in emacs by 
ispell-region command).

   * Grammar mistakes, e.g `Flag means need track subreg live range for 
the allocno`.  I understand English is not your native languages (as for 
me).  In case of some doubts I'd recommend to check grammar in ChatGPT 
(Proofread: <english> text).

   * Some local variables use upper case letters (e.g. `int A`) which 
should be used for macros or enums according to GNU coding standard 
(https://www.gnu.org/prep/standards/standards.html) .

   * Sometimes you put one space at the end of sentence.  Please see GNU 
coding standard and GCC coding conventions 
(https://gcc.gnu.org/codingconventions.html)

   * There is no uniformity in your code, e.g. sometimes you use 'i++', 
sometimes `++i` or `i += 1`.  Although the uniformity is not necessary, 
it makes a better impression about the patches.


I also did not find what targets did you use for testing.  I am asking 
this because I see new testsuite failures (apx-spill_to_egprs-1.c) even 
on x86-64.  It might be nothing as the test expects a specific code 
generation.

Also besides testing major targets I'd recommend testing at least one 
big endian target (I'd recommend ppc64be. gcc110.fsfrance.org could be 
used for this).  Plenty RA issues occur because BE targets are not tested.