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

[Lehua Ding <lehua.ding@rivai.ai>]

Hi Vladimir,

On 2023/11/14 3:37, Vladimir Makarov wrote:
> 
> 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.

Sorry for these issue, I'll address all those comments.

> 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.

There was testing x86, aarch64, riscv not long ago, but it looks like 
I'm missing something, I just locally tested with the latest code and 
also reproduced this fail you mentioned, along with a c++ fail 
(pr106877.C). I'll have a look at the cause.

> 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.

You said the address looks a bit wrong, it should be this 
gcc110.fsffrance.org right? I looked for it and it looks like you have 
to go to portal.cfarm.net first to apply for an account on this site, 
I'll try that, thanks a lot.

-- 
Best,
Lehua (RiVAI)
lehua.ding@rivai.ai