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.d...@rivai.ai
