[PATCH v2] [testsuite] Fixup dg-options in {gcc, g++, gfortran}.dg/vect.exp tests
Changes in v2:
- Better changelog entry.
- NFC.
This patch has been tested on
- aarch64-linux-gnu
- arm-linux-gnueabihf (VFP, NEON disabled by default),
- arm-none-eabi (Soft-FP)
with the following [expected] differences in the test results:
- FAIL now PASS [FAIL => PASS]:
Executed from: gcc:gcc.dg/vect/vect.exp
gcc:gcc.dg/vect/vect.exp=gcc.dg/vect/pr113576.c (test for excess errors)
gcc:gcc.dg/vect/vect.exp=gcc.dg/vect/pr113576.c -flto -ffat-lto-objects
(test for excess errors)
- UNSUPPORTED disappears[UNSUP=> ]:
Executed from: g++:g++.dg/vect/vect.exp
g++:g++.dg/vect/vect.exp=g++.dg/vect/pr84556.cc -std=gnu++98
- UNSUPPORTED appears [ =>UNSUP]:
Executed from: g++:g++.dg/vect/vect.exp
g++:g++.dg/vect/vect.exp=g++.dg/vect/pr84556.cc -std=c++98
- UNRESOLVED disappears [UNRES=> ]:
Executed from: gcc:gcc.dg/vect/vect.exp
gcc:gcc.dg/vect/vect.exp=gcc.dg/vect/pr113576.c -flto -ffat-lto-objects
compilation failed to produce executable
gcc:gcc.dg/vect/vect.exp=gcc.dg/vect/pr113576.c compilation failed to
produce executable
This patch was motivated by gcc.dg/vect/pr113576.c, which currently
fails to compile for ARM targets without NEON.
=== CUT ===
Testsuites driven by vect.exp rely on check_vect_support_and_set_flags
to set appropriate DEFAULT_VECTFLAGS for a given target (e.g., add
-mfpu=neon for arm-linux-gnueabi). Unfortunately, these flags are
overwritten by dg-options directive, which can cause tests to fail.
Behavior of dg-options is documented in vect.exp files, but not
all developers look at the .exp file when adding a new testcase.
This caused a few dg-options directives to be used instead of
the more appropriate dg-additional-options.
This patch changes target-independent dg-options into
dg-additional-options. This patch does not touch target-specific
dg-options and target-specific tests to avoid disturbing the gentle
balance of target-specific vectorization.
This patch also removes a couple of unneeded "dg-do run" directives
to avoid failures on compile-only targets. Default action is, again,
set by check_vect_support_and_set_flags.
Lastly, I avoided renaming tests that use -O options to O-*
filename format because this support is not consistent between
gcc.dg/vect/, g++.dg/vect/, and gfortran.dg/vect/ testsuites.
It seems dg-additional-options is cleaner.
This patch does the following,
1. do not change target-specific tests, e.g., gcc.dg/vect/costmodel/riscv/*;
2. do not change { dg-options FOO { target { target-*-pattern } } };
3. do not remove { dg-do run { target { target-*-pattern } } };
4. change { dg-options FOO } to { dg-additional-options FOO };
5. remove { dg-do run } in several tests, where it is clearly not needed.
gcc/testsuite/ChangeLog:
PR testsuite/114307
* gcc.dg/vect/vect-cond-reduc-in-order-2-signed-zero.c: Remove dg-run.
* gcc.dg/vect/complex/complex-operations-run.c: Likewise.
* gcc.dg/vect/pr113576.c: Remove dg-run. Use dg-additional-options for
test-specific flags.
* gcc.dg/vect/gimplefe-40.c: Use dg-additional-options for
test-specific flags.
* gcc.dg/vect/gimplefe-41.c: Likewise.
* gcc.dg/vect/pr101145inf.c: Likewise.
* gcc.dg/vect/pr101145inf_1.c: Likewise.
* gcc.dg/vect/pr108316.c: Likewise.
* gcc.dg/vect/pr109011-1.c: Likewise.
* gcc.dg/vect/pr109011-2.c: Likewise.
* gcc.dg/vect/pr109011-3.c: Likewise.
* gcc.dg/vect/pr109011-4.c: Likewise.
* gcc.dg/vect/pr109011-5.c: Likewise.
* gcc.dg/vect/pr111846.c: Likewise.
* gcc.dg/vect/pr111860-2.c: Likewise.
* gcc.dg/vect/pr111860-3.c: Likewise.
* gcc.dg/vect/pr113002.c: Likewise.
* gcc.dg/vect/pr84711.c: Likewise.
* gcc.dg/vect/pr85597.c: Likewise.
* gcc.dg/vect/pr88497-1.c: Likewise.
* gcc.dg/vect/pr88497-2.c: Likewise.
* gcc.dg/vect/pr88497-3.c: Likewise.
* gcc.dg/vect/pr88497-4.c: Likewise.
* gcc.dg/vect/pr88497-5.c: Likewise.
* gcc.dg/vect/pr88497-7.c: Likewise.
* gcc.dg/vect/pr92347.c: Likewise.
* gcc.dg/vect/pr93069.c: Likewise.
* gcc.dg/vect/pr97241.c: Likewise.
* gcc.dg/vect/pr99102.c: Likewise.
* gcc.dg/vect/vect-early-break_65.c: Likewise.
* gcc.dg/vect/vect-fold-1.c: Likewise.
* gcc.dg/vect/vect-ifcvt-19.c: Likewise.
* gcc.dg/vect/vect-ifcvt-20.c: Likewise.
* gcc.dg/vect/vect-reduc-epilogue-gaps.c: Likewise.
* gcc.dg/vect/vect-singleton_1.c: Likewise.
* g++.dg/vect/pr84556.cc: Likewise.
* gfortran.dg/vect/fast-math-mgrid-resid.f: Likewise.
* gfortran.dg/vect/pr77848.f: Likewise.
* gfortran.dg/vect/pr90913.f90: Likewise.
---
gcc/testsuite/g++.dg/vect/pr84556.cc | 2 +-
Re: [PATCH] [testsuite] Fixup dg-options in {gcc, g++, gfortran}.dg/vect.exp tests
> On Mar 13, 2024, at 15:25, Richard Earnshaw
> wrote:
>
>
>
> On 13/03/2024 10:58, Maxim Kuvyrkov wrote:
>> This patch has been tested on
>> - aarch64-linux-gnu
>> - arm-linux-gnueabihf (VFP, NEON disabled by default),
>> - arm-none-eabi (Soft-FP)
>> with the following [expected] differences in the test results:
>> - FAIL now PASS [FAIL => PASS]:
>> Executed from: gcc:gcc.dg/vect/vect.exp
>> gcc:gcc.dg/vect/vect.exp=gcc.dg/vect/pr113576.c (test for excess errors)
>> gcc:gcc.dg/vect/vect.exp=gcc.dg/vect/pr113576.c -flto -ffat-lto-objects
>> (test for excess errors)
>> - UNSUPPORTED disappears[UNSUP=> ]:
>> Executed from: g++:g++.dg/vect/vect.exp
>> g++:g++.dg/vect/vect.exp=g++.dg/vect/pr84556.cc -std=gnu++98
>> - UNSUPPORTED appears [ =>UNSUP]:
>> Executed from: g++:g++.dg/vect/vect.exp
>> g++:g++.dg/vect/vect.exp=g++.dg/vect/pr84556.cc -std=c++98
>> - UNRESOLVED disappears [UNRES=> ]:
>> Executed from: gcc:gcc.dg/vect/vect.exp
>> gcc:gcc.dg/vect/vect.exp=gcc.dg/vect/pr113576.c -flto -ffat-lto-objects
>> compilation failed to produce executable
>> gcc:gcc.dg/vect/vect.exp=gcc.dg/vect/pr113576.c compilation failed to
>> produce executable
>> This patch was motivated by gcc.dg/vect/pr113576.c, which currently
>> fails to compile for ARM targets without NEON.
>> === CUT ===
>> Testsuites driven by vect.exp rely on check_vect_support_and_set_flags
>> to set appropriate DEFAULT_VECTFLAGS for a given target (e.g., add
>> -mfpu=neon for arm-linux-gnueabi). Unfortunately, these flags are
>> overwritten by dg-options directive, which can cause tests to fail.
>> Behavior of dg-options is documented in vect.exp files, but not
>> all developers look at the .exp file when adding a new testcase.
>> This caused a few dg-options directives to be used instead of
>> the more appropriate dg-additional-options.
>> This patch changes target-independent dg-options into
>> dg-additional-options. This patch does not touch target-specific
>> dg-options and target-specific tests to avoid disturbing the gentle
>> balance of target-specific vectorization.
>> This patch also removes a couple of unneeded "dg-do run" directives
>> to avoid failures on compile-only targets. Default action is, again,
>> set by check_vect_support_and_set_flags.
>> Lastly, I avoided renaming tests that use -O options to O-*
>> filename format because this support is not consistent between
>> gcc.dg/vect/, g++.dg/vect/, and gfortran.dg/vect/ testsuites.
>> It seems dg-additional-options is cleaner.
>> This patch does the following,
>> 1. do not change target-specific tests, e.g., gcc.dg/vect/costmodel/riscv/*;
>> 2. do not change { dg-options FOO { target { target-*-pattern } } };
>> 3. do not remove { dg-do run { target { target-*-pattern } } };
>> 4. change { dg-options FOO } to { dg-additional-options FOO };
>> 5. remove { dg-do run } in several tests, where it is clearly not needed.
>> gcc/testsuite/ChangeLog:
>> PR testsuite/114307
>> * g++.dg/vect/pr84556.cc: Fixup.
>> * gcc.dg/vect/complex/complex-operations-run.c Fixup.
>> * gcc.dg/vect/gimplefe-40.c Fixup.
>> * gcc.dg/vect/gimplefe-41.c Fixup.
>> * gcc.dg/vect/pr101145inf.c Fixup.
>> * gcc.dg/vect/pr101145inf_1.c Fixup.
>> * gcc.dg/vect/pr108316.c Fixup.
>> * gcc.dg/vect/pr109011-1.c Fixup.
>> * gcc.dg/vect/pr109011-2.c Fixup.
>> * gcc.dg/vect/pr109011-3.c Fixup.
>> * gcc.dg/vect/pr109011-4.c Fixup.
>> * gcc.dg/vect/pr109011-5.c Fixup.
>> * gcc.dg/vect/pr111846.c Fixup.
>> * gcc.dg/vect/pr111860-2.c Fixup.
>> * gcc.dg/vect/pr111860-3.c Fixup.
>> * gcc.dg/vect/pr113002.c Fixup.
>> * gcc.dg/vect/pr113576.c Fixup.
>> * gcc.dg/vect/pr84711.c Fixup.
>> * gcc.dg/vect/pr85597.c Fixup.
>> * gcc.dg/vect/pr88497-1.c Fixup.
>> * gcc.dg/vect/pr88497-2.c Fixup.
>> * gcc.dg/vect/pr88497-3.c Fixup.
>> * gcc.dg/vect/pr88497-4.c Fixup.
>> * gcc.dg/vect/pr88497-5.c Fixup.
>> * gcc.dg/vect/pr88497-7.c Fixup.
>> * gcc.dg/vect/pr92347.c Fixup.
>> * gcc.dg/vect/pr93069.c Fixup.
>> * gcc.dg/vect/pr97241.c Fixup.
>> * gcc.dg/vect/pr99102.c Fixup.
>> * gcc.dg/vect/vect-cond-reduc-in-order-2-signed-zero.c Fixup.
>> * gcc.dg/vect/vect-early-break_65.c Fixup.
>> * gcc.dg/vect/vect-fold-1.c Fixup.
>> * gcc.dg/vect/vect-ifcvt-19.c Fixup.
>> * gcc.dg/vect/vect-ifcvt-20.c Fixup.
>> * gcc.dg/vect/vect-reduc-epilogue-gaps.c Fixup.
>> * g
[PATCH] [testsuite] Fixup dg-options in {gcc, g++, gfortran}.dg/vect.exp tests
This patch has been tested on
- aarch64-linux-gnu
- arm-linux-gnueabihf (VFP, NEON disabled by default),
- arm-none-eabi (Soft-FP)
with the following [expected] differences in the test results:
- FAIL now PASS [FAIL => PASS]:
Executed from: gcc:gcc.dg/vect/vect.exp
gcc:gcc.dg/vect/vect.exp=gcc.dg/vect/pr113576.c (test for excess errors)
gcc:gcc.dg/vect/vect.exp=gcc.dg/vect/pr113576.c -flto -ffat-lto-objects
(test for excess errors)
- UNSUPPORTED disappears[UNSUP=> ]:
Executed from: g++:g++.dg/vect/vect.exp
g++:g++.dg/vect/vect.exp=g++.dg/vect/pr84556.cc -std=gnu++98
- UNSUPPORTED appears [ =>UNSUP]:
Executed from: g++:g++.dg/vect/vect.exp
g++:g++.dg/vect/vect.exp=g++.dg/vect/pr84556.cc -std=c++98
- UNRESOLVED disappears [UNRES=> ]:
Executed from: gcc:gcc.dg/vect/vect.exp
gcc:gcc.dg/vect/vect.exp=gcc.dg/vect/pr113576.c -flto -ffat-lto-objects
compilation failed to produce executable
gcc:gcc.dg/vect/vect.exp=gcc.dg/vect/pr113576.c compilation failed to
produce executable
This patch was motivated by gcc.dg/vect/pr113576.c, which currently
fails to compile for ARM targets without NEON.
=== CUT ===
Testsuites driven by vect.exp rely on check_vect_support_and_set_flags
to set appropriate DEFAULT_VECTFLAGS for a given target (e.g., add
-mfpu=neon for arm-linux-gnueabi). Unfortunately, these flags are
overwritten by dg-options directive, which can cause tests to fail.
Behavior of dg-options is documented in vect.exp files, but not
all developers look at the .exp file when adding a new testcase.
This caused a few dg-options directives to be used instead of
the more appropriate dg-additional-options.
This patch changes target-independent dg-options into
dg-additional-options. This patch does not touch target-specific
dg-options and target-specific tests to avoid disturbing the gentle
balance of target-specific vectorization.
This patch also removes a couple of unneeded "dg-do run" directives
to avoid failures on compile-only targets. Default action is, again,
set by check_vect_support_and_set_flags.
Lastly, I avoided renaming tests that use -O options to O-*
filename format because this support is not consistent between
gcc.dg/vect/, g++.dg/vect/, and gfortran.dg/vect/ testsuites.
It seems dg-additional-options is cleaner.
This patch does the following,
1. do not change target-specific tests, e.g., gcc.dg/vect/costmodel/riscv/*;
2. do not change { dg-options FOO { target { target-*-pattern } } };
3. do not remove { dg-do run { target { target-*-pattern } } };
4. change { dg-options FOO } to { dg-additional-options FOO };
5. remove { dg-do run } in several tests, where it is clearly not needed.
gcc/testsuite/ChangeLog:
PR testsuite/114307
* g++.dg/vect/pr84556.cc: Fixup.
* gcc.dg/vect/complex/complex-operations-run.c Fixup.
* gcc.dg/vect/gimplefe-40.c Fixup.
* gcc.dg/vect/gimplefe-41.c Fixup.
* gcc.dg/vect/pr101145inf.c Fixup.
* gcc.dg/vect/pr101145inf_1.c Fixup.
* gcc.dg/vect/pr108316.c Fixup.
* gcc.dg/vect/pr109011-1.c Fixup.
* gcc.dg/vect/pr109011-2.c Fixup.
* gcc.dg/vect/pr109011-3.c Fixup.
* gcc.dg/vect/pr109011-4.c Fixup.
* gcc.dg/vect/pr109011-5.c Fixup.
* gcc.dg/vect/pr111846.c Fixup.
* gcc.dg/vect/pr111860-2.c Fixup.
* gcc.dg/vect/pr111860-3.c Fixup.
* gcc.dg/vect/pr113002.c Fixup.
* gcc.dg/vect/pr113576.c Fixup.
* gcc.dg/vect/pr84711.c Fixup.
* gcc.dg/vect/pr85597.c Fixup.
* gcc.dg/vect/pr88497-1.c Fixup.
* gcc.dg/vect/pr88497-2.c Fixup.
* gcc.dg/vect/pr88497-3.c Fixup.
* gcc.dg/vect/pr88497-4.c Fixup.
* gcc.dg/vect/pr88497-5.c Fixup.
* gcc.dg/vect/pr88497-7.c Fixup.
* gcc.dg/vect/pr92347.c Fixup.
* gcc.dg/vect/pr93069.c Fixup.
* gcc.dg/vect/pr97241.c Fixup.
* gcc.dg/vect/pr99102.c Fixup.
* gcc.dg/vect/vect-cond-reduc-in-order-2-signed-zero.c Fixup.
* gcc.dg/vect/vect-early-break_65.c Fixup.
* gcc.dg/vect/vect-fold-1.c Fixup.
* gcc.dg/vect/vect-ifcvt-19.c Fixup.
* gcc.dg/vect/vect-ifcvt-20.c Fixup.
* gcc.dg/vect/vect-reduc-epilogue-gaps.c Fixup.
* gcc.dg/vect/vect-singleton_1.c Fixup.
* gfortran.dg/vect/fast-math-mgrid-resid.f Fixup.
* gfortran.dg/vect/pr77848.f Fixup.
* gfortran.dg/vect/pr90913.f90 Fixup.
---
gcc/testsuite/g++.dg/vect/pr84556.cc | 2 +-
gcc/testsuite/gcc.dg/vect/complex/complex-operations-run.c | 1 -
gcc/testsuite/gcc.dg/vect/gimplefe-40.c| 2 +-
gcc/testsuite/gcc.dg/vect/gimplefe-41.c| 2 +-
gcc/testsuite/gcc.dg/vect/pr101145inf.c| 2 +-
gcc/testsuite/gcc.dg/vect/pr101145inf_1.c | 2 +-
Re: [PATCH v1 08/13] aarch64: Add Cygwin and MinGW environments for AArch64
> On Feb 29, 2024, at 21:59, Richard Earnshaw (lists) > wrote: > > On 29/02/2024 17:55, Andrew Pinski (QUIC) wrote: >>> -Original Message- >>> From: Maxim Kuvyrkov >>> Sent: Thursday, February 29, 2024 9:46 AM >>> To: Andrew Pinski (QUIC) >>> Cc: Evgeny Karpov ; Andrew Pinski >>> ; Richard Sandiford ; gcc- >>> patc...@gcc.gnu.org; 10wa...@gmail.com; m...@harmstone.com; Zac >>> Walker ; Ron Riddle >>> ; Radek Barton >>> Subject: Re: [PATCH v1 08/13] aarch64: Add Cygwin and MinGW >>> environments for AArch64 >>> >>> WARNING: This email originated from outside of Qualcomm. Please be wary >>> of any links or attachments, and do not enable macros. >>> >>>> On Feb 29, 2024, at 21:35, Andrew Pinski (QUIC) >>> wrote: >>>> >>>> >>>> >>>>> -----Original Message- >>>>> From: Evgeny Karpov >>>>> Sent: Thursday, February 29, 2024 8:46 AM >>>>> To: Andrew Pinski >>>>> Cc: Richard Sandiford ; gcc- >>>>> patc...@gcc.gnu.org; 10wa...@gmail.com; Maxim Kuvyrkov >>>>> ; m...@harmstone.com; Zac Walker >>>>> ; Ron Riddle ; >>>>> Radek Barton ; Andrew Pinski (QUIC) >>>>> >>>>> Subject: [PATCH v1 08/13] aarch64: Add Cygwin and MinGW environments >>>>> for AArch64 >>>>> >>>>> Wednesday, February 28, 2024 2:00 AM >>>>> Andrew Pinski wrote: >>>>> >>>>>> What does this mean with respect to C++ exceptions? Or you using >>>>>> SJLJ exceptions support or the dwarf unwinding ones without SEH >>> support? >>>>>> I am not sure if SJLJ exceptions is well tested any more in GCC either. >>>>>> >>>>>> Also I have a question if you ran the full GCC/G++ testsuites and >>>>>> what were the results? >>>>>> If you did run it, did you use a cross compiler or the native >>>>>> compiler? Did you do a bootstrap (GCC uses C++ but no exceptions >>> though)? >>>>> >>>>> As mentioned in the cover letter and the thread, the current >>>>> contribution covers only the C scope. >>>>> Exception handling is fully disabled for now. >>>>> There is an experimental build with C++ and SEH, however, it is not >>>>> included in the plan for the current contribution. >>>>> >>>>> https://github.com/Windows-on-ARM-Experiments/mingw-woarm64- >>> build >>>>> >>>>>> If you run using a cross compiler, did you use ssh or some other >>>>>> route to run the applications? >>>>>> >>>>>> Thanks, >>>>>> Andrew Pinski >>>>> >>>>> GitHub Actions are used to cross-compile toolchains, packages and >>>>> tests, and execute tests on Windows Arm64. >>>> >>>> This does not answer my question because what you are running is just >>> simple testcases and not the FULL GCC testsuite. >>>> So again have you ran the GCC testsuite and do you have a dejagnu board to >>> be able to execute the binaries? >>>> I think without the GCC testsuite ran to find all of the known failures, >>>> you are >>> going to be running into many issues. >>>> The GCC testsuite includes many tests for ABI corner cases and many >>> features that you will most likely not think about testing using your simple >>> testcases. >>>> In fact I suspect there will be some of the aarch64 testcases which will >>>> need >>> to be modified for the windows ABI which you have not done yet. >>> >>> Hi Andrew, >>> >>> We (Linaro) have a prototype CI loop setup for testing aarch64-w64- >>> mingw32, and we have results for gcc-c and libatomic -- see [1]. >>> >>> The results are far from clean, but that's expected. This patch series >>> aims at >>> enabling C hello-world only, and subsequent patch series will improve the >>> state of the port. >>> >>> [1] https://ci.linaro.org/job/tcwg_gnu_mingw_check_gcc--master-woa64- >>> build/6/artifact/artifacts/sumfiles/ >> >> Looking at these results, this port is not in any shape or form to be >> upstreamed right now. Even simple -g will cause failures. >> Note we don't need a clean testsuite run but the patch series is not even >> allowing enabling hello world due to the -g not being able to used. >> > > It seemed to me as though the patch was posted for comments, not for > immediate inclusion. I agree this isn't ready for committing yet, but > neither should the submitters wait until it's perfect before posting it. > > I think it's gcc-15 material, so now is about the right time to be thinking > about it. Hi Andrew, I agree with Richard. This patch series is large as is, and it has clear goals: 1. Enable aarch64-w64-mingw32 to compile C-language hello-world. 2. Not regress any other targets. As far as I know, it achieves both, but both Microsoft and Linaro will do additional testing on x86_64-w64-mingw32 to confirm. There are more features and fixes for aarch64-w64-mingw32 waiting in the development repos on github, but I don't see any point in cleaning them up and preparing for submission before this already-quiet-large patchset if reviewed. Thank you, -- Maxim Kuvyrkov https://www.linaro.org
Re: [PATCH v1 08/13] aarch64: Add Cygwin and MinGW environments for AArch64
> On Feb 29, 2024, at 21:35, Andrew Pinski (QUIC) > wrote: > > > >> -Original Message- >> From: Evgeny Karpov >> Sent: Thursday, February 29, 2024 8:46 AM >> To: Andrew Pinski >> Cc: Richard Sandiford ; gcc- >> patc...@gcc.gnu.org; 10wa...@gmail.com; Maxim Kuvyrkov >> ; m...@harmstone.com; Zac Walker >> ; Ron Riddle ; Radek >> Barton ; Andrew Pinski (QUIC) >> >> Subject: [PATCH v1 08/13] aarch64: Add Cygwin and MinGW environments >> for AArch64 >> >> Wednesday, February 28, 2024 2:00 AM >> Andrew Pinski wrote: >> >>> What does this mean with respect to C++ exceptions? Or you using SJLJ >>> exceptions support or the dwarf unwinding ones without SEH support? >>> I am not sure if SJLJ exceptions is well tested any more in GCC either. >>> >>> Also I have a question if you ran the full GCC/G++ testsuites and what >>> were the results? >>> If you did run it, did you use a cross compiler or the native >>> compiler? Did you do a bootstrap (GCC uses C++ but no exceptions though)? >> >> As mentioned in the cover letter and the thread, the current contribution >> covers only the C scope. >> Exception handling is fully disabled for now. >> There is an experimental build with C++ and SEH, however, it is not included >> in >> the plan for the current contribution. >> >> https://github.com/Windows-on-ARM-Experiments/mingw-woarm64-build >> >>> If you run using a cross compiler, did you use ssh or some other route >>> to run the applications? >>> >>> Thanks, >>> Andrew Pinski >> >> GitHub Actions are used to cross-compile toolchains, packages and tests, and >> execute tests on Windows Arm64. > > This does not answer my question because what you are running is just simple > testcases and not the FULL GCC testsuite. > So again have you ran the GCC testsuite and do you have a dejagnu board to be > able to execute the binaries? > I think without the GCC testsuite ran to find all of the known failures, you > are going to be running into many issues. > The GCC testsuite includes many tests for ABI corner cases and many features > that you will most likely not think about testing using your simple testcases. > In fact I suspect there will be some of the aarch64 testcases which will need > to be modified for the windows ABI which you have not done yet. Hi Andrew, We (Linaro) have a prototype CI loop setup for testing aarch64-w64-mingw32, and we have results for gcc-c and libatomic -- see [1]. The results are far from clean, but that's expected. This patch series aims at enabling C hello-world only, and subsequent patch series will improve the state of the port. [1] https://ci.linaro.org/job/tcwg_gnu_mingw_check_gcc--master-woa64-build/6/artifact/artifacts/sumfiles/ Thanks, -- Maxim Kuvyrkov https://www.linaro.org
Re: [PATCH v1 00/13] Add aarch64-w64-mingw32 target
Hi Evgeny, Great job! For reference, here is a test build of this patch series using Linaro Toolchain CI: https://ci.linaro.org/view/tcwg-build/job/tcwg_gnu_mingw_build--master-woa64-build/9/artifact/artifacts/ -- Maxim Kuvyrkov https://www.linaro.org > On Feb 21, 2024, at 21:47, Evgeny Karpov wrote: > > Hello, > > We would like to take your attention to the review of changes for the > new GCC target, aarch64-w64-mingw32. The new target will be > supported, tested, added to CI, and maintained by Linaro. This marks > the first of three planned patch series contributing to the GCC C > compiler's support for Windows Arm64. > > 1. Minimal aarch64-w64-mingw32 C implementation to cross-compile > hello-world with libgcc for Windows Arm64 using MinGW. > 2. Extension of the aarch64-w64-mingw32 C implementation to > cross-compile OpenSSL, OpenBLAS, FFmpeg, and libjpeg-turbo. All > packages successfully pass tests. > 3. Addition of call stack support for debugging, resolution of > optimization issues in the C compiler, and DLL export/import for the > aarch64-w64-mingw32 target. > > This patch series introduces the 1st point, which involves building > hello-world for the aarch64-w64-mingw32 target. The patch depends on > the binutils changes for the aarch64-w64-mingw32 target that have > already been merged. > > The binutils should include recent relocation fixes. > f87eaf8ff3995a5888c6dc4996a20c770e6bcd36 > aarch64: Add new relocations and limit COFF AArch64 relocation offsets > > The series is structured in a way to trivially show that it should not > affect any other targets. > > In this patch, several changes have been made to support the > aarch64-w64-mingw32 target for GCC. The modifications include the > definition of the MS ABI for aarch64, adjustments to FIXED_REGISTERS > and STATIC_CHAIN_REGNUM for different ABIs, and specific definitions > for COFF format on AArch64. Additionally, the patch reuses MinGW > types and definitions from i386, relocating them to a new > mingw folder for shared usage between both targets. > > MinGW-specific options have been introduced for AArch64, along with > override options for aarch64-w64-mingw32. Builtin stack probing for > override options for aarch64-w64-mingw32. Builtin stack probing for > AArch64 has been enabled as an alternative for chkstk. Symbol name > encoding and section information handling for aarch64-w64-mingw32 have > been incorporated, and the MinGW environment has been added, which > will also be utilized for defining the Cygwin environment in the > future. > > The patch includes renaming "x86 Windows Options" to "Cygwin and MinGW > Options," which now encompasses AArch64 as well. AArch64-specific > Cygwin and MinGW Options have been introduced for the unique > requirements of the AArch64 architecture. > > Function type declaration and named sections support have been added. > The necessary objects for Cygwin and MinGW have been built for the > aarch64-w64-mingw32 target, and relevant files such as msformat-c.cc > and winnt-d.cc have been moved to the mingw folder for reuse in > AArch64. > > Furthermore, the aarch64-w64-mingw32 target has been included in both > libatomic and libgcc, ensuring support for the AArch64 architecture > within these libraries. These changes collectively enhance the > capabilities of GCC for the specified target. > > Coauthors: Zac Walker , > Mark Harmstone and > Ron Riddle > > Refactored, prepared, and validated by > Radek Barton and > Evgeny Karpov > > Special thanks to the Linaro GNU toolchain team for internal review > and assistance in preparing the patch series! > > Regards, > Evgeny > > > Zac Walker (13): > Introduce aarch64-w64-mingw32 target > aarch64: The aarch64-w64-mingw32 target implements the MS ABI > aarch64: Mark x18 register as a fixed register for MS ABI > aarch64: Add aarch64-w64-mingw32 COFF > Reuse MinGW from i386 for AArch64 > Rename section and encoding functions from i386 which will be used in >aarch64 > Exclude i386 functionality from aarch64 build > aarch64: Add Cygwin and MinGW environments for AArch64 > aarch64: Add SEH to machine_function > Rename "x86 Windows Options" to "Cygwin and MinGW Options" > aarch64: Build and add objects for Cygwin and MinGW for AArch64 > aarch64: Add aarch64-w64-mingw32 target to libatomic > Add aarch64-w64-mingw32 target to libgcc > > fixincludes/mkfixinc.sh | 3 +- > gcc/config.gcc| 47 +++-- > gcc/config/aarch64/aarch64-coff.h | 92 + > gcc/config/aarch64/aarch64-opts.h | 7 + > gcc/config/aarch64/aar
Re: [PATCH] tree-optimization/113576 - non-empty latch and may_be_zero vectorization
> On Jan 24, 2024, at 18:40, Richard Biener wrote:
>
> We can't support niters with may_be_zero when we end up with a
> non-empty latch due to early exit peeling. At least not in
> the simplistic way the vectorizer handles this now. Disallow
> it again for exits that are not the last one.
>
> Bootstrap and regtest running on x86_64-unknown-linux-gnu.
>
> PR tree-optimization/113576
> * tree-vect-loop.cc (vec_init_loop_exit_info): Only allow
> exits with may_be_zero niters when its the last one.
>
> * gcc.dg/vect/pr113576.c: New testcase.
> ---
> gcc/testsuite/gcc.dg/vect/pr113576.c | 157 +++
> gcc/tree-vect-loop.cc| 9 +-
> 2 files changed, 164 insertions(+), 2 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/vect/pr113576.c
>
> diff --git a/gcc/testsuite/gcc.dg/vect/pr113576.c
> b/gcc/testsuite/gcc.dg/vect/pr113576.c
> new file mode 100644
> index 000..da5ddb09e33
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/vect/pr113576.c
> @@ -0,0 +1,157 @@
> +/* { dg-do run } */
> +/* { dg-options "-O3" } */
> +/* { dg-additional-options "-march=skylake-avx512" } */
Please adjust the testcase; this fails on non-x86_64 targets, see [1] and [2].
[1]
https://patchwork.sourceware.org/project/gcc/patch/20240124144159.51c503858...@sourceware.org/
[2]
https://ci.linaro.org/job/tcwg_gcc_check--master-aarch64-precommit/4765/artifact/artifacts/artifacts.precommit/00-sumfiles/gcc.log.1.xz
Thanks!
--
Maxim Kuvyrkov
https://www.linaro.org
[PATCH] Make gcc.target/arm/bics_3.c testcase a bit more generic [PR113542]
After fwprop improvement in r14-8319-g86de9b66480, codegen in
bics_3.c test changed from "bics" to "bic" instruction, with
the overall instruction stream remaining at the same quality.
This patch makes the scan-assembler directive accept both
"bics" and "bic".
BEFORE r14-8319-g86de9b66480:
bicsr0, r0, r1 @ 9 [c=4 l=4] *andsi_notsi_si_compare0_scratch
mov r0, #1 @ 23[c=4 l=4] *thumb2_movsi_vfp/1
it eq
moveq r0, #0 @ 26[c=8 l=4] *p *thumb2_movsi_vfp/2
bx lr @ 29[c=8 l=4] *thumb2_return
AFTER r14-8319-g86de9b66480:
bic r0, r0, r1 @ 8 [c=4 l=4] andsi_notsi_si
subsr0, r0, #0 @ 22[c=4 l=4] cmpsi2_addneg/0
it ne
movne r0, #1 @ 23[c=8 l=4] *p *thumb2_movsi_vfp/2
bx lr @ 26[c=8 l=4] *thumb2_return
gcc/testsuite/ChangeLog:
PR target/113542
* gcc.target/arm/bics_3.c: Update scan-assembler directive.
---
gcc/testsuite/gcc.target/arm/bics_3.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/gcc/testsuite/gcc.target/arm/bics_3.c
b/gcc/testsuite/gcc.target/arm/bics_3.c
index e056b264e15..c5bed3c92d2 100644
--- a/gcc/testsuite/gcc.target/arm/bics_3.c
+++ b/gcc/testsuite/gcc.target/arm/bics_3.c
@@ -35,6 +35,6 @@ main (void)
return 0;
}
-/* { dg-final { scan-assembler-times "bics\tr\[0-9\]+, r\[0-9\]+, r\[0-9\]+" 2
} } */
-/* { dg-final { scan-assembler-times "bics\tr\[0-9\]+, r\[0-9\]+, r\[0-9\]+,
.sl #2" 1 } } */
+/* { dg-final { scan-assembler-times "bics?\tr\[0-9\]+, r\[0-9\]+, r\[0-9\]+"
2 } } */
+/* { dg-final { scan-assembler-times "bics?\tr\[0-9\]+, r\[0-9\]+, r\[0-9\]+,
.sl #2" 1 } } */
--
2.34.1
Re: [PATCH v3 1/8] sched-deps.cc (find_modifiable_mems): Avoid exponential behavior
> On Jan 17, 2024, at 19:05, Maxim Kuvyrkov wrote: > >> On Jan 17, 2024, at 19:02, Richard Biener wrote: >> >> On Wed, Jan 17, 2024 at 8:39 AM Maxim Kuvyrkov >> wrote: >>> >>>> On Jan 17, 2024, at 10:51, Richard Biener >>>> wrote: >>>> >>>> On Tue, Jan 16, 2024 at 3:52 PM Jeff Law wrote: >>>>> >>>>> >>>>> >>>>> On 1/15/24 05:56, Maxim Kuvyrkov wrote: >>>>>> Hi Vladimir, >>>>>> Hi Jeff, >>>>>> >>>>>> Richard and Alexander have reviewed this patch and [I assume] have no >>>>>> further comments. OK to merge? >>>>> I think the question is whether or not we're too late. I know that >>>>> Richard S has held off on his late-combine pass and I'm holding off on >>>>> the ext-dce work due to the fact that we're well past stage1 close. >>>>> >>>>> I think the release managers ought to have the final say on this. >>>> >>>> I'm fine with this now, it doesn't change code generation. >>> >>> Thanks, Richard. >>> >>> I'll merge the fix for PR96388 and PR111554 -- patch 1/8. I'll commit >>> cleanups and improvements to scheduler logging -- patches 2/8 - 8/8 -- when >>> stage1 opens. >> >> This seems to have caused a compare-debug bootstrap issue on x86_64-linux, >> >> gcc/fortran/f95-lang.o differs >> >> does n_mem_deps or n_inc_deps include debug insns? > > Thanks, investigating. Hi Richard, Yes, both n_mem_deps or n_inc_deps include debug insns, I posted a patch for this in https://gcc.gnu.org/pipermail/gcc-patches/2024-January/643267.html . Testing it now. If you prefer, I can revert the fix for PR96388 and PR111554. Kind regards, -- Maxim Kuvyrkov https://www.linaro.org
[PATCH v1] Fix compare-debug bootstrap failure
... caused by scheduler fix for PR96388 and PR111554.
This patch adjusts decision sched-deps.cc:find_inc() to use
length of dependency lists sans any DEBUG_INSN instructions.
gcc/ChangeLog:
* haifa-sched.cc (dep_list_size): Make global.
* sched-deps.cc (find_inc): Use instead of sd_lists_size().
* sched-int.h (dep_list_size): Declare.
---
gcc/haifa-sched.cc | 8 ++--
gcc/sched-deps.cc | 6 +++---
gcc/sched-int.h| 2 ++
3 files changed, 11 insertions(+), 5 deletions(-)
diff --git a/gcc/haifa-sched.cc b/gcc/haifa-sched.cc
index 49ee589aed7..1bc610f9a5f 100644
--- a/gcc/haifa-sched.cc
+++ b/gcc/haifa-sched.cc
@@ -1560,8 +1560,7 @@ contributes_to_priority_p (dep_t dep)
}
/* Compute the number of nondebug deps in list LIST for INSN. */
-
-static int
+int
dep_list_size (rtx_insn *insn, sd_list_types_def list)
{
sd_iterator_def sd_it;
@@ -1571,6 +1570,11 @@ dep_list_size (rtx_insn *insn, sd_list_types_def list)
if (!MAY_HAVE_DEBUG_INSNS)
return sd_lists_size (insn, list);
+ /* TODO: We should split normal and debug insns into separate SD_LIST_*
+ sub-lists, and then we'll be able to use something like
+ sd_lists_size(insn, list & SD_LIST_NON_DEBUG)
+ instead of walking dependencies below. */
+
FOR_EACH_DEP (insn, list, sd_it, dep)
{
if (DEBUG_INSN_P (DEP_CON (dep)))
diff --git a/gcc/sched-deps.cc b/gcc/sched-deps.cc
index 0615007c560..5034e664e5e 100644
--- a/gcc/sched-deps.cc
+++ b/gcc/sched-deps.cc
@@ -4791,7 +4791,7 @@ find_inc (struct mem_inc_info *mii, bool backwards)
sd_iterator_def sd_it;
dep_t dep;
sd_list_types_def mem_deps = backwards ? SD_LIST_HARD_BACK : SD_LIST_FORW;
- int n_mem_deps = sd_lists_size (mii->mem_insn, mem_deps);
+ int n_mem_deps = dep_list_size (mii->mem_insn, mem_deps);
sd_it = sd_iterator_start (mii->mem_insn, mem_deps);
while (sd_iterator_cond (_it, ))
@@ -4808,12 +4808,12 @@ find_inc (struct mem_inc_info *mii, bool backwards)
if (backwards)
{
inc_cand = pro;
- n_inc_deps = sd_lists_size (inc_cand, SD_LIST_BACK);
+ n_inc_deps = dep_list_size (inc_cand, SD_LIST_BACK);
}
else
{
inc_cand = con;
- n_inc_deps = sd_lists_size (inc_cand, SD_LIST_FORW);
+ n_inc_deps = dep_list_size (inc_cand, SD_LIST_FORW);
}
/* In the FOR_EACH_DEP loop below we will create additional n_inc_deps
diff --git a/gcc/sched-int.h b/gcc/sched-int.h
index ab784fe0d17..4df092013e9 100644
--- a/gcc/sched-int.h
+++ b/gcc/sched-int.h
@@ -1677,6 +1677,8 @@ extern void sd_copy_back_deps (rtx_insn *, rtx_insn *,
bool);
extern void sd_delete_dep (sd_iterator_def);
extern void sd_debug_lists (rtx, sd_list_types_def);
+extern int dep_list_size (rtx_insn *, sd_list_types_def);
+
/* Macros and declarations for scheduling fusion. */
#define FUSION_MAX_PRIORITY (INT_MAX)
extern bool sched_fusion;
--
2.34.1
Re: [PATCH v3 1/8] sched-deps.cc (find_modifiable_mems): Avoid exponential behavior
> On Jan 17, 2024, at 19:02, Richard Biener wrote: > > On Wed, Jan 17, 2024 at 8:39 AM Maxim Kuvyrkov > wrote: >> >>> On Jan 17, 2024, at 10:51, Richard Biener >>> wrote: >>> >>> On Tue, Jan 16, 2024 at 3:52 PM Jeff Law wrote: >>>> >>>> >>>> >>>> On 1/15/24 05:56, Maxim Kuvyrkov wrote: >>>>> Hi Vladimir, >>>>> Hi Jeff, >>>>> >>>>> Richard and Alexander have reviewed this patch and [I assume] have no >>>>> further comments. OK to merge? >>>> I think the question is whether or not we're too late. I know that >>>> Richard S has held off on his late-combine pass and I'm holding off on >>>> the ext-dce work due to the fact that we're well past stage1 close. >>>> >>>> I think the release managers ought to have the final say on this. >>> >>> I'm fine with this now, it doesn't change code generation. >> >> Thanks, Richard. >> >> I'll merge the fix for PR96388 and PR111554 -- patch 1/8. I'll commit >> cleanups and improvements to scheduler logging -- patches 2/8 - 8/8 -- when >> stage1 opens. > > This seems to have caused a compare-debug bootstrap issue on x86_64-linux, > > gcc/fortran/f95-lang.o differs > > does n_mem_deps or n_inc_deps include debug insns? Thanks, investigating. -- Maxim Kuvyrkov https://www.linaro.org
Re: [PATCH v3 1/8] sched-deps.cc (find_modifiable_mems): Avoid exponential behavior
> On Jan 17, 2024, at 10:51, Richard Biener wrote: > > On Tue, Jan 16, 2024 at 3:52 PM Jeff Law wrote: >> >> >> >> On 1/15/24 05:56, Maxim Kuvyrkov wrote: >>> Hi Vladimir, >>> Hi Jeff, >>> >>> Richard and Alexander have reviewed this patch and [I assume] have no >>> further comments. OK to merge? >> I think the question is whether or not we're too late. I know that >> Richard S has held off on his late-combine pass and I'm holding off on >> the ext-dce work due to the fact that we're well past stage1 close. >> >> I think the release managers ought to have the final say on this. > > I'm fine with this now, it doesn't change code generation. Thanks, Richard. I'll merge the fix for PR96388 and PR111554 -- patch 1/8. I'll commit cleanups and improvements to scheduler logging -- patches 2/8 - 8/8 -- when stage1 opens. Regards, -- Maxim Kuvyrkov https://www.linaro.org
Re: [PATCH v3 8/8] Improve logging of scheduler dependency analysis context
Dear scheduler maintainers,
Gentle ping. This patch improves debugging output, it does not touch
scheduling logic.
On Wed, 22 Nov 2023 at 15:15, Maxim Kuvyrkov
wrote:
> Scheduler dependency analysis uses two main data structures:
> 1. reg_pending_* data contains effects of INSN on the register file,
>which is then incorporated into ...
> 2. deps_desc object, which contains commulative information about all
>instructions processed from deps_desc object's initialization.
>
> This patch adds debug dumping of (2).
>
> Dependency analysis contexts (aka deps_desc objects) are huge, but
> each instruction affects only a small amount of data in these objects.
> Therefore, it is most useful to dump differential information
> compared to the dependency state after previous instruction.
>
> gcc/ChangeLog:
>
> * sched-deps.cc (reset_deps, dump_rtx_insn_list)
> (rtx_insn_list_same_p): New helper functions.
> (dump_deps_desc_diff): New function to dump dependency information.
> (sched_analysis_prev_deps): New static variable.
> (sched_analyze_insn): Dump dependency information.
> (init_deps_global, finish_deps_global): Handle
> sched_analysis_prev_deps.
> * sched-int.h (struct deps_reg): Update comments.
> * sched-rgn.cc (concat_insn_list, concat_expr_list): Update
> comments.
> ---
> gcc/sched-deps.cc | 197 ++
> gcc/sched-int.h | 9 ++-
> gcc/sched-rgn.cc | 5 ++
> 3 files changed, 210 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/sched-deps.cc b/gcc/sched-deps.cc
> index f9290c82fd2..edca9927e23 100644
> --- a/gcc/sched-deps.cc
> +++ b/gcc/sched-deps.cc
> @@ -1677,6 +1677,15 @@ delete_all_dependences (rtx_insn *insn)
> sd_delete_dep (sd_it);
> }
>
> +/* Re-initialize existing dependency context DEPS to be a copy of FROM.
> */
> +static void
> +reset_deps (class deps_desc *deps, class deps_desc *from)
> +{
> + free_deps (deps);
> + init_deps (deps, false);
> + deps_join (deps, from);
> +}
> +
> /* All insns in a scheduling group except the first should only have
> dependencies on the previous insn in the group. So we find the
> first instruction in the scheduling group by walking the dependence
> @@ -2960,6 +2969,177 @@ dump_reg_pending_data (FILE *file, rtx_insn *insn)
> }
> }
>
> +/* Dump rtx_insn_list LIST.
> + Consider moving to lists.cc if there are users outside of
> sched-deps.cc. */
> +static void
> +dump_rtx_insn_list (FILE *file, rtx_insn_list *list)
> +{
> + for (; list; list = list->next ())
> +fprintf (file, " %d", INSN_UID (list->insn ()));
> +}
> +
> +/* Return TRUE if lists A and B have same elements in the same order. */
> +static bool
> +rtx_insn_list_same_p (rtx_insn_list *a, rtx_insn_list *b)
> +{
> + for (; a && b; a = a->next (), b = b->next ())
> +if (a->insn () != b->insn ())
> + return false;
> +
> + if (a || b)
> +return false;
> +
> + return true;
> +}
> +
> +/* Dump parts of DEPS that are different from PREV.
> + Dumping all information from dependency context produces huge
> + hard-to-analize logs; differential dumping is way more managable. */
> +static void
> +dump_deps_desc_diff (FILE *file, class deps_desc *deps, class deps_desc
> *prev)
> +{
> + /* Each "paragraph" is a single line of output. */
> +
> + /* Note on param_max_pending_list_length:
> + During normal dependency analysis various lists should not exceed
> this
> + limit. Searching for "!!!" in scheduler logs can point to potential
> bugs
> + or poorly-handled corner-cases. */
> +
> + if (!rtx_insn_list_same_p (deps->pending_read_insns,
> +prev->pending_read_insns))
> +{
> + fprintf (file, ";; deps pending mem reads length(%d):",
> + deps->pending_read_list_length);
> + if ((deps->pending_read_list_length +
> deps->pending_write_list_length)
> + >= param_max_pending_list_length)
> + fprintf (file, "%d insns!!!", deps->pending_read_list_length);
> + else
> + dump_rtx_insn_list (file, deps->pending_read_insns);
> + fprintf (file, "\n");
> +}
> +
> + if (!rtx_insn_list_same_p (deps->pending_write_insns,
> +prev->pending_write_insns))
> +{
> + fprintf (file, ";; deps pending mem writes length(%d):",
> + deps->pending_write_list_length);
> + if ((deps->pending_read_list_length +
> deps->pending_wri
Re: [PATCH v3 7/8] Improve logging of register data in scheduler dependency analysis
Dear scheduler maintainers,
Gentle ping. This patch improves debugging output, it does not touch
scheduling logic.
On Wed, 22 Nov 2023 at 15:15, Maxim Kuvyrkov
wrote:
> Scheduler dependency analysis uses two main data structures:
> 1. reg_pending_* data contains effects of INSN on the register file,
>which is then incorporated into ...
> 2. deps_desc object, which contains commulative information about all
>instructions processed from deps_desc object's initialization.
>
> This patch adds debug dumping of (1).
>
> gcc/ChangeLog:
>
> * sched-deps.cc (print-rtl.h): Include for str_pattern_slim().
> (dump_reg_pending_data): New function.
> (sched_analyze_insn): Use it.
> ---
> gcc/sched-deps.cc | 90 ++-
> 1 file changed, 89 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/sched-deps.cc b/gcc/sched-deps.cc
> index e0d3c97d935..f9290c82fd2 100644
> --- a/gcc/sched-deps.cc
> +++ b/gcc/sched-deps.cc
> @@ -38,6 +38,7 @@ along with GCC; see the file COPYING3. If not see
> #include "sched-int.h"
> #include "cselib.h"
> #include "function-abi.h"
> +#include "print-rtl.h"
>
> #ifdef INSN_SCHEDULING
>
> @@ -432,10 +433,24 @@ dep_spec_p (dep_t dep)
>return false;
> }
>
> +/* These regsets describe how a single instruction affects registers.
> + Their "life-time" is restricted to a single call of
> sched_analyze_insn().
> + They are populated by sched_analyze_1() and sched_analyze_2(), and
> + then sched_analyze_insn() transfers data from these into
> deps->reg_last[i].
> + Near the end sched_analyze_insn() clears these regsets for the next
> + insn. */
> static regset reg_pending_sets;
> static regset reg_pending_clobbers;
> static regset reg_pending_uses;
> static regset reg_pending_control_uses;
> +
> +/* Similar to reg_pending_* regsets, this variable specifies whether
> + the current insn analyzed by sched_analyze_insn() is a scheduling
> + barrier that should "split" dependencies inside a block. Internally
> + sched-deps.cc does this by pretending that the barrier insn uses and
> + sets all registers.
> + Near the end sched_analyze_insn() transfers barrier info from this
> variable
> + into deps->last_reg_pending_barrier. */
> static enum reg_pending_barrier_mode reg_pending_barrier;
>
> /* Hard registers implicitly clobbered or used (or may be implicitly
> @@ -2880,7 +2895,77 @@ get_implicit_reg_pending_clobbers (HARD_REG_SET
> *temp, rtx_insn *insn)
>*temp &= ~ira_no_alloc_regs;
> }
>
> -/* Analyze an INSN with pattern X to find all dependencies. */
> +/* Dump state of reg_pending_* data for debug purposes.
> + Dump only non-empty data to reduce log clobber. */
> +static void
> +dump_reg_pending_data (FILE *file, rtx_insn *insn)
> +{
> + fprintf (file, "\n");
> + fprintf (file, ";; sched_analysis after insn %d: %s\n",
> + INSN_UID (insn), str_pattern_slim (PATTERN (insn)));
> +
> + if (!REG_SET_EMPTY_P (reg_pending_sets)
> + || !REG_SET_EMPTY_P (reg_pending_clobbers)
> + || !REG_SET_EMPTY_P (reg_pending_uses)
> + || !REG_SET_EMPTY_P (reg_pending_control_uses))
> +{
> + fprintf (file, ";; insn reg");
> + if (!REG_SET_EMPTY_P (reg_pending_sets))
> + {
> + fprintf (file, " sets(");
> + dump_regset (reg_pending_sets, file);
> + fprintf (file, ")");
> + }
> + if (!REG_SET_EMPTY_P (reg_pending_clobbers))
> + {
> + fprintf (file, " clobbers(");
> + dump_regset (reg_pending_clobbers, file);
> + fprintf (file, ")");
> + }
> + if (!REG_SET_EMPTY_P (reg_pending_uses))
> + {
> + fprintf (file, " uses(");
> + dump_regset (reg_pending_uses, file);
> + fprintf (file, ")");
> + }
> + if (!REG_SET_EMPTY_P (reg_pending_control_uses))
> + {
> + fprintf (file, " control(");
> + dump_regset (reg_pending_control_uses, file);
> + fprintf (file, ")");
> + }
> + fprintf (file, "\n");
> +}
> +
> + if (reg_pending_barrier)
> +fprintf (file, ";; insn reg barrier: %d\n", reg_pending_barrier);
> +
> + if (!hard_reg_set_empty_p (implicit_reg_pending_clobbers)
> + || !hard_reg_set_empty_p (implicit_reg_pending_uses))
> +{
> + fprintf (file, ";; insn reg");
> + if (!hard_reg_set_empty_p (implicit_reg_pending_clobbers))
> + {
> +
Re: [PATCH v3 6/8] sched_deps.cc: Simplify initialization of dependency contexts
Dear scheduler maintainers,
Gentle ping. This is a trivial cleanup.
On Wed, 22 Nov 2023 at 15:14, Maxim Kuvyrkov
wrote:
> gcc/ChangeLog:
>
> * sched-deps.cc (init_deps, init_deps_reg_last): Simplify.
> (free_deps): Remove useless code.
> ---
> gcc/sched-deps.cc | 13 -
> 1 file changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/gcc/sched-deps.cc b/gcc/sched-deps.cc
> index 2a87158ba4b..e0d3c97d935 100644
> --- a/gcc/sched-deps.cc
> +++ b/gcc/sched-deps.cc
> @@ -3927,10 +3927,9 @@ init_deps (class deps_desc *deps, bool
> lazy_reg_last)
>int max_reg = (reload_completed ? FIRST_PSEUDO_REGISTER : max_reg_num
> ());
>
>deps->max_reg = max_reg;
> - if (lazy_reg_last)
> -deps->reg_last = NULL;
> - else
> -deps->reg_last = XCNEWVEC (struct deps_reg, max_reg);
> + deps->reg_last = NULL;
> + if (!lazy_reg_last)
> +init_deps_reg_last (deps);
>INIT_REG_SET (>reg_last_in_use);
>
>deps->pending_read_insns = 0;
> @@ -3961,9 +3960,7 @@ init_deps (class deps_desc *deps, bool lazy_reg_last)
> void
> init_deps_reg_last (class deps_desc *deps)
> {
> - gcc_assert (deps && deps->max_reg > 0);
> - gcc_assert (deps->reg_last == NULL);
> -
> + gcc_assert (deps && deps->max_reg > 0 && deps->reg_last == NULL);
>deps->reg_last = XCNEWVEC (struct deps_reg, deps->max_reg);
> }
>
> @@ -4013,8 +4010,6 @@ free_deps (class deps_desc *deps)
> it at all. */
>free (deps->reg_last);
>deps->reg_last = NULL;
> -
> - deps = NULL;
> }
>
> /* Remove INSN from dependence contexts DEPS. */
> --
> 2.34.1
>
>
--
Maxim Kuvyrkov
www.linaro.org
Re: [PATCH v3 5/8] Add a bit more logging scheduler's dependency analysis
Dear scheduler maintainers,
Gentle ping. This is a trivial patch, which makes debugging sched-deps.cc
slightly more enjoyable.
On Wed, 22 Nov 2023 at 15:14, Maxim Kuvyrkov
wrote:
> gcc/ChangeLog:
>
> * sched-deps.cc (sd_add_dep, find_inc): Add logging about
> dependency creation.
> ---
> gcc/sched-deps.cc | 30 ++
> 1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/sched-deps.cc b/gcc/sched-deps.cc
> index 4d357079a7a..2a87158ba4b 100644
> --- a/gcc/sched-deps.cc
> +++ b/gcc/sched-deps.cc
> @@ -1342,6 +1342,13 @@ sd_add_dep (dep_t dep, bool resolved_p)
> in the bitmap caches of dependency information. */
>if (true_dependency_cache != NULL)
> set_dependency_caches (dep);
> +
> + if (sched_verbose >= 9)
> +{
> + fprintf (sched_dump, "created dependency ");
> + dump_dep (sched_dump, dep, 1);
> + fprintf (sched_dump, "\n");
> +}
> }
>
> /* Add or update backward dependence between INSN and ELEM
> @@ -4879,18 +4886,33 @@ find_inc (struct mem_inc_info *mii, bool backwards)
> we create. */
> gcc_assert (mii->inc_insn == inc_cand);
>
> + int n_deps_created = 0;
> if (backwards)
> {
> FOR_EACH_DEP (mii->inc_insn, SD_LIST_BACK, sd_it, dep)
> - add_dependence_1 (mii->mem_insn, DEP_PRO (dep),
> - REG_DEP_TRUE);
> + {
> + add_dependence_1 (mii->mem_insn, DEP_PRO (dep),
> + REG_DEP_TRUE);
> + ++n_deps_created;
> + }
> }
> else
> {
> FOR_EACH_DEP (mii->inc_insn, SD_LIST_FORW, sd_it, dep)
> - add_dependence_1 (DEP_CON (dep), mii->mem_insn,
> - REG_DEP_ANTI);
> + {
> + add_dependence_1 (DEP_CON (dep), mii->mem_insn,
> + REG_DEP_ANTI);
> + ++n_deps_created;
> + }
> }
> + if (sched_verbose >= 6)
> + fprintf (sched_dump,
> +"created %d deps for mem_insn %d due to "
> +"inc_insn %d %s deps\n",
> + n_deps_created, INSN_UID (mii->mem_insn),
> +INSN_UID (mii->inc_insn),
> +backwards ? "backward" : "forward");
> +
> return true;
> }
> next:
> --
> 2.34.1
>
>
--
Maxim Kuvyrkov
www.linaro.org
Re: [PATCH v3 4/8] Improve and fix sched-deps.cc: dump_dep() and dump_lists().
Dear scheduler maintainers,
Gentle ping. This patch is borderline trivial and affects only the lucky
few who debug sched-deps.cc code. OK to merge?
On Wed, 22 Nov 2023 at 15:14, Maxim Kuvyrkov
wrote:
> Better propagate flags from dump_lists() into dump_dep() and
> add a missing "]" in dump_lists().
>
> gcc/ChangeLog:
>
> * sched-deps.cc (DUMP_DEP_PRO): Improve comment.
> (dump_dep_flags): Remove.
> (DUMP_LISTS_SIZE, DUMP_LISTS_DEPS, DUMP_LISTS_ALL): Continue
> numbering from DUMP_DEP_* flags.
> (dump_lists): Update and fix.
> ---
> gcc/sched-deps.cc | 21 +++--
> 1 file changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/gcc/sched-deps.cc b/gcc/sched-deps.cc
> index 005fc0f567e..4d357079a7a 100644
> --- a/gcc/sched-deps.cc
> +++ b/gcc/sched-deps.cc
> @@ -132,7 +132,8 @@ static void dump_ds (FILE *, ds_t);
> /* Define flags for dump_dep (). */
>
> /* Dump producer of the dependence. */
> -#define DUMP_DEP_PRO (2)
> +#define DUMP_DEP_PRO (2) /* Reserve "1" for handling of DUMP_DEP_ALL and
> + DUMP_LISTS_ALL. */
>
> /* Dump consumer of the dependence. */
> #define DUMP_DEP_CON (4)
> @@ -206,9 +207,6 @@ dump_dep (FILE *dump, dep_t dep, int flags)
>fprintf (dump, ">");
> }
>
> -/* Default flags for dump_dep (). */
> -static int dump_dep_flags = (DUMP_DEP_PRO | DUMP_DEP_CON);
> -
> /* Dump all fields of DEP to STDERR. */
> void
> sd_debug_dep (dep_t dep)
> @@ -1454,19 +1452,20 @@ sd_delete_dep (sd_iterator_def sd_it)
> }
>
> /* Dump size of the lists. */
> -#define DUMP_LISTS_SIZE (2)
> +#define DUMP_LISTS_SIZE (32) /* (DUMP_DEP_STATUS << 1) */
>
> /* Dump dependencies of the lists. */
> -#define DUMP_LISTS_DEPS (4)
> +#define DUMP_LISTS_DEPS (64)
>
> /* Dump all information about the lists. */
> #define DUMP_LISTS_ALL (DUMP_LISTS_SIZE | DUMP_LISTS_DEPS)
>
> /* Dump deps_lists of INSN specified by TYPES to DUMP.
> - FLAGS is a bit mask specifying what information about the lists needs
> - to be printed.
> + FLAGS is a bit mask specifying what information about the lists and
> + the individual deps needs to be printed, this is a combination of
> + DUMP_DEP_* and DUMP_LISTS_* flags.
> If FLAGS has the very first bit set, then dump all information about
> - the lists and propagate this bit into the callee dump functions. */
> + the lists and deps propagate this bit into the callee dump functions.
> */
> static void
> dump_lists (FILE *dump, rtx insn, sd_list_types_def types, int flags)
> {
> @@ -1488,10 +1487,12 @@ dump_lists (FILE *dump, rtx insn,
> sd_list_types_def types, int flags)
> {
>FOR_EACH_DEP (insn, types, sd_it, dep)
> {
> - dump_dep (dump, dep, dump_dep_flags | all);
> + dump_dep (dump, dep, flags | all);
> fprintf (dump, " ");
> }
> }
> +
> + fprintf (dump, "]");
> }
>
> /* Dump all information about deps_lists of INSN specified by TYPES
> --
> 2.34.1
>
>
--
Maxim Kuvyrkov
www.linaro.org
Re: [PATCH v3 3/8] Simplify handling of INSN_ and EXPR_LISTs in sched-rgn.cc
Dear RTL maintainers,
Gently ping. This patch adds a couple of new functions to lists.cc, which
then are used to simplify logic in the scheduler. OK to merge?
On Wed, 22 Nov 2023 at 15:14, Maxim Kuvyrkov
wrote:
> This patch simplifies logic behind deps_join(), which will be
> important for the upcoming improvements of sched-deps.cc logging.
>
> The only functional change is that when deps_join() is called with
> empty state for the 2nd argument, it will not reverse INSN_ and
> EXPR_LISTs in the 1st argument. Before this patch the lists were
> reversed due to use of concat_*_LIST(). Now, with copy_*_LIST()
> used for this case, the lists will remain in the original order.
>
> gcc/ChangeLog:
>
> * lists.cc (copy_EXPR_LIST, concat_EXPR_LIST): New functions.
> * rtl.h (copy_EXPR_LIST, concat_EXPR_LIST): Declare.
> * sched-rgn.cc (concat_insn_list, concat_expr_list): New helpers.
> (concat_insn_mem_list): Simplify.
> (deps_join): Update
> ---
> gcc/lists.cc | 30 +++-
> gcc/rtl.h| 4 +++-
> gcc/sched-rgn.cc | 51 +++-
> 3 files changed, 61 insertions(+), 24 deletions(-)
>
> diff --git a/gcc/lists.cc b/gcc/lists.cc
> index 2cdf37ad533..83e7bf32176 100644
> --- a/gcc/lists.cc
> +++ b/gcc/lists.cc
> @@ -160,6 +160,24 @@ free_INSN_LIST_list (rtx_insn_list **listp)
>free_list ((rtx *)listp, _insn_list);
> }
>
> +/* Make a copy of the EXPR_LIST list LINK and return it. */
> +rtx_expr_list *
> +copy_EXPR_LIST (rtx_expr_list *link)
> +{
> + rtx_expr_list *new_queue;
> + rtx_expr_list **pqueue = _queue;
> +
> + for (; link; link = link->next ())
> +{
> + rtx x = link->element ();
> + rtx_expr_list *newlink = alloc_EXPR_LIST (REG_NOTE_KIND (link), x,
> NULL);
> + *pqueue = newlink;
> + pqueue = (rtx_expr_list **) (newlink, 1);
> +}
> + *pqueue = NULL;
> + return new_queue;
> +}
> +
> /* Make a copy of the INSN_LIST list LINK and return it. */
> rtx_insn_list *
> copy_INSN_LIST (rtx_insn_list *link)
> @@ -178,12 +196,22 @@ copy_INSN_LIST (rtx_insn_list *link)
>return new_queue;
> }
>
> +/* Duplicate the EXPR_LIST elements of COPY and prepend them to OLD. */
> +rtx_expr_list *
> +concat_EXPR_LIST (rtx_expr_list *copy, rtx_expr_list *old)
> +{
> + rtx_expr_list *new_rtx = old;
> + for (; copy; copy = copy->next ())
> +new_rtx = alloc_EXPR_LIST (REG_NOTE_KIND (copy), copy->element (),
> new_rtx);
> + return new_rtx;
> +}
> +
> /* Duplicate the INSN_LIST elements of COPY and prepend them to OLD. */
> rtx_insn_list *
> concat_INSN_LIST (rtx_insn_list *copy, rtx_insn_list *old)
> {
>rtx_insn_list *new_rtx = old;
> - for (; copy ; copy = copy->next ())
> + for (; copy; copy = copy->next ())
> {
>new_rtx = alloc_INSN_LIST (copy->insn (), new_rtx);
>PUT_REG_NOTE_KIND (new_rtx, REG_NOTE_KIND (copy));
> diff --git a/gcc/rtl.h b/gcc/rtl.h
> index e4b6cc0dbb5..7e952d7cbeb 100644
> --- a/gcc/rtl.h
> +++ b/gcc/rtl.h
> @@ -3764,10 +3764,12 @@ extern void free_EXPR_LIST_list (rtx_expr_list **);
> extern void free_INSN_LIST_list (rtx_insn_list **);
> extern void free_EXPR_LIST_node (rtx);
> extern void free_INSN_LIST_node (rtx);
> +extern rtx_expr_list *alloc_EXPR_LIST (int, rtx, rtx);
> extern rtx_insn_list *alloc_INSN_LIST (rtx, rtx);
> +extern rtx_expr_list *copy_EXPR_LIST (rtx_expr_list *);
> extern rtx_insn_list *copy_INSN_LIST (rtx_insn_list *);
> +extern rtx_expr_list *concat_EXPR_LIST (rtx_expr_list *, rtx_expr_list *);
> extern rtx_insn_list *concat_INSN_LIST (rtx_insn_list *, rtx_insn_list *);
> -extern rtx_expr_list *alloc_EXPR_LIST (int, rtx, rtx);
> extern void remove_free_INSN_LIST_elem (rtx_insn *, rtx_insn_list **);
> extern rtx remove_list_elem (rtx, rtx *);
> extern rtx_insn *remove_free_INSN_LIST_node (rtx_insn_list **);
> diff --git a/gcc/sched-rgn.cc b/gcc/sched-rgn.cc
> index e5964f54ead..da3ec0458ff 100644
> --- a/gcc/sched-rgn.cc
> +++ b/gcc/sched-rgn.cc
> @@ -2585,25 +2585,32 @@ add_branch_dependences (rtx_insn *head, rtx_insn
> *tail)
>
> static class deps_desc *bb_deps;
>
> +/* Return a new insn_list with all the elements from the two input
> lists. */
> +static rtx_insn_list *
> +concat_insn_list (rtx_insn_list *copy, rtx_insn_list *old)
> +{
> + if (!old)
> +return copy_INSN_LIST (copy);
> + return concat_INSN_LIST (copy, old);
> +}
> +
> +/* Return a new expr_list with all the elements from the two input
> lists. */
> +static rtx_expr_list *
> +concat_expr_list (rtx_expr_list *copy, rtx_expr_list *old)
>
Re: [PATCH v3 1/8] sched-deps.cc (find_modifiable_mems): Avoid exponential behavior
Hi Vladimir,
Hi Jeff,
Richard and Alexander have reviewed this patch and [I assume] have no
further comments. OK to merge?
On Wed, 22 Nov 2023 at 15:14, Maxim Kuvyrkov
wrote:
> This patch avoids sched-deps.cc:find_inc() creating exponential number
> of dependencies, which become memory and compilation time hogs.
> Consider example (simplified from PR96388) ...
> ===
> sp=sp-4 // sp_insnA
> mem_insnA1[sp+A1]
> ...
> mem_insnAN[sp+AN]
> sp=sp-4 // sp_insnB
> mem_insnB1[sp+B1]
> ...
> mem_insnBM[sp+BM]
> ===
>
> [For simplicity, let's assume find_inc(backwards==true)].
> In this example find_modifiable_mems() will arrange for mem_insnA*
> to be able to pass sp_insnA, and, while doing this, will create
> dependencies between all mem_insnA*s and sp_insnB -- because sp_insnB
> is a consumer of sp_insnA. After this sp_insnB will have N new
> backward dependencies.
> Then find_modifiable_mems() gets to mem_insnB*s and starts to create
> N new dependencies for _every_ mem_insnB*. This gets us N*M new
> dependencies.
>
> In PR96833's testcase N and M are 10k-15k, which causes RAM usage of
> 30GB and compilation time of 30 minutes, with sched2 accounting for
> 95% of both metrics. After this patch the RAM usage is down to 1GB
> and compilation time is down to 3-4 minutes, with sched2 no longer
> standing out on -ftime-report or memory usage.
>
> gcc/ChangeLog:
>
> PR rtl-optimization/96388
> PR rtl-optimization/111554
> * sched-deps.cc (find_inc): Avoid exponential behavior.
> ---
> gcc/sched-deps.cc | 48 +++
> 1 file changed, 44 insertions(+), 4 deletions(-)
>
> diff --git a/gcc/sched-deps.cc b/gcc/sched-deps.cc
> index c23218890f3..005fc0f567e 100644
> --- a/gcc/sched-deps.cc
> +++ b/gcc/sched-deps.cc
> @@ -4779,24 +4779,59 @@ parse_add_or_inc (struct mem_inc_info *mii,
> rtx_insn *insn, bool before_mem)
> /* Once a suitable mem reference has been found and the corresponding data
> in MII has been filled in, this function is called to find a suitable
> add or inc insn involving the register we found in the memory
> - reference. */
> + reference.
> + If successful, this function will create additional dependencies
> between
> + - mii->inc_insn's producers and mii->mem_insn as a consumer (if
> backwards)
> + - mii->inc_insn's consumers and mii->mem_insn as a producer (if
> !backwards).
> +*/
>
> static bool
> find_inc (struct mem_inc_info *mii, bool backwards)
> {
>sd_iterator_def sd_it;
>dep_t dep;
> + sd_list_types_def mem_deps = backwards ? SD_LIST_HARD_BACK :
> SD_LIST_FORW;
> + int n_mem_deps = sd_lists_size (mii->mem_insn, mem_deps);
>
> - sd_it = sd_iterator_start (mii->mem_insn,
> -backwards ? SD_LIST_HARD_BACK : SD_LIST_FORW);
> + sd_it = sd_iterator_start (mii->mem_insn, mem_deps);
>while (sd_iterator_cond (_it, ))
> {
>dep_node_t node = DEP_LINK_NODE (*sd_it.linkp);
>rtx_insn *pro = DEP_PRO (dep);
>rtx_insn *con = DEP_CON (dep);
> - rtx_insn *inc_cand = backwards ? pro : con;
> + rtx_insn *inc_cand;
> + int n_inc_deps;
> +
>if (DEP_NONREG (dep) || DEP_MULTIPLE (dep))
> goto next;
> +
> + if (backwards)
> + {
> + inc_cand = pro;
> + n_inc_deps = sd_lists_size (inc_cand, SD_LIST_BACK);
> + }
> + else
> + {
> + inc_cand = con;
> + n_inc_deps = sd_lists_size (inc_cand, SD_LIST_FORW);
> + }
> +
> + /* In the FOR_EACH_DEP loop below we will create additional
> n_inc_deps
> +for mem_insn. This by itself is not a problem, since each
> mem_insn
> +will have only a few inc_insns associated with it. However, if
> +we consider that a single inc_insn may have a lot of mem_insns,
> AND,
> +on top of that, a few other inc_insns associated with it --
> +those _other inc_insns_ will get (n_mem_deps * number of MEM
> insns)
> +dependencies created for them. This may cause an exponential
> +growth of memory usage and scheduling time.
> +See PR96388 for details.
> +We [heuristically] use n_inc_deps as a proxy for the number of MEM
> +insns, and drop opportunities for breaking modifiable_mem
> dependencies
> +when dependency lists grow beyond reasonable size. */
> + if (n_mem_deps * n_inc_deps
> + >= param_max_pending_list_length * param_max_pending_list_length)
> + goto next;
> +
>if (parse_add_or_inc (mii, inc_cand, backwards))
> {
> struct dep_r
[PATCH v3 7/8] Improve logging of register data in scheduler dependency analysis
Scheduler dependency analysis uses two main data structures:
1. reg_pending_* data contains effects of INSN on the register file,
which is then incorporated into ...
2. deps_desc object, which contains commulative information about all
instructions processed from deps_desc object's initialization.
This patch adds debug dumping of (1).
gcc/ChangeLog:
* sched-deps.cc (print-rtl.h): Include for str_pattern_slim().
(dump_reg_pending_data): New function.
(sched_analyze_insn): Use it.
---
gcc/sched-deps.cc | 90 ++-
1 file changed, 89 insertions(+), 1 deletion(-)
diff --git a/gcc/sched-deps.cc b/gcc/sched-deps.cc
index e0d3c97d935..f9290c82fd2 100644
--- a/gcc/sched-deps.cc
+++ b/gcc/sched-deps.cc
@@ -38,6 +38,7 @@ along with GCC; see the file COPYING3. If not see
#include "sched-int.h"
#include "cselib.h"
#include "function-abi.h"
+#include "print-rtl.h"
#ifdef INSN_SCHEDULING
@@ -432,10 +433,24 @@ dep_spec_p (dep_t dep)
return false;
}
+/* These regsets describe how a single instruction affects registers.
+ Their "life-time" is restricted to a single call of sched_analyze_insn().
+ They are populated by sched_analyze_1() and sched_analyze_2(), and
+ then sched_analyze_insn() transfers data from these into deps->reg_last[i].
+ Near the end sched_analyze_insn() clears these regsets for the next
+ insn. */
static regset reg_pending_sets;
static regset reg_pending_clobbers;
static regset reg_pending_uses;
static regset reg_pending_control_uses;
+
+/* Similar to reg_pending_* regsets, this variable specifies whether
+ the current insn analyzed by sched_analyze_insn() is a scheduling
+ barrier that should "split" dependencies inside a block. Internally
+ sched-deps.cc does this by pretending that the barrier insn uses and
+ sets all registers.
+ Near the end sched_analyze_insn() transfers barrier info from this variable
+ into deps->last_reg_pending_barrier. */
static enum reg_pending_barrier_mode reg_pending_barrier;
/* Hard registers implicitly clobbered or used (or may be implicitly
@@ -2880,7 +2895,77 @@ get_implicit_reg_pending_clobbers (HARD_REG_SET *temp,
rtx_insn *insn)
*temp &= ~ira_no_alloc_regs;
}
-/* Analyze an INSN with pattern X to find all dependencies. */
+/* Dump state of reg_pending_* data for debug purposes.
+ Dump only non-empty data to reduce log clobber. */
+static void
+dump_reg_pending_data (FILE *file, rtx_insn *insn)
+{
+ fprintf (file, "\n");
+ fprintf (file, ";; sched_analysis after insn %d: %s\n",
+ INSN_UID (insn), str_pattern_slim (PATTERN (insn)));
+
+ if (!REG_SET_EMPTY_P (reg_pending_sets)
+ || !REG_SET_EMPTY_P (reg_pending_clobbers)
+ || !REG_SET_EMPTY_P (reg_pending_uses)
+ || !REG_SET_EMPTY_P (reg_pending_control_uses))
+{
+ fprintf (file, ";; insn reg");
+ if (!REG_SET_EMPTY_P (reg_pending_sets))
+ {
+ fprintf (file, " sets(");
+ dump_regset (reg_pending_sets, file);
+ fprintf (file, ")");
+ }
+ if (!REG_SET_EMPTY_P (reg_pending_clobbers))
+ {
+ fprintf (file, " clobbers(");
+ dump_regset (reg_pending_clobbers, file);
+ fprintf (file, ")");
+ }
+ if (!REG_SET_EMPTY_P (reg_pending_uses))
+ {
+ fprintf (file, " uses(");
+ dump_regset (reg_pending_uses, file);
+ fprintf (file, ")");
+ }
+ if (!REG_SET_EMPTY_P (reg_pending_control_uses))
+ {
+ fprintf (file, " control(");
+ dump_regset (reg_pending_control_uses, file);
+ fprintf (file, ")");
+ }
+ fprintf (file, "\n");
+}
+
+ if (reg_pending_barrier)
+fprintf (file, ";; insn reg barrier: %d\n", reg_pending_barrier);
+
+ if (!hard_reg_set_empty_p (implicit_reg_pending_clobbers)
+ || !hard_reg_set_empty_p (implicit_reg_pending_uses))
+{
+ fprintf (file, ";; insn reg");
+ if (!hard_reg_set_empty_p (implicit_reg_pending_clobbers))
+ {
+ print_hard_reg_set (file, implicit_reg_pending_clobbers,
+ " implicit clobbers(", false);
+ fprintf (file, ")");
+ }
+ if (!hard_reg_set_empty_p (implicit_reg_pending_uses))
+ {
+ print_hard_reg_set (file, implicit_reg_pending_uses,
+ " implicit uses(", false);
+ fprintf (file, ")");
+ }
+ fprintf (file, "\n");
+}
+}
+
+/* Analyze an INSN with pattern X to find all dependencies.
+ This analysis uses two main data structures:
+ 1. reg_pending_* data contains effects of INSN on the register file,
+ which is then incorporated into ...
+ 2. deps_desc object, which contains commulative information about all
+ instructions processed from deps_desc object's initialization. */
static void
sched_analyze_insn (class deps_desc *deps, rtx x, rtx_insn *insn)
{
@@ -3328,6 +3413,9 @@ sched_analyze_insn (class
[PATCH v3 4/8] Improve and fix sched-deps.cc: dump_dep() and dump_lists().
Better propagate flags from dump_lists() into dump_dep() and
add a missing "]" in dump_lists().
gcc/ChangeLog:
* sched-deps.cc (DUMP_DEP_PRO): Improve comment.
(dump_dep_flags): Remove.
(DUMP_LISTS_SIZE, DUMP_LISTS_DEPS, DUMP_LISTS_ALL): Continue
numbering from DUMP_DEP_* flags.
(dump_lists): Update and fix.
---
gcc/sched-deps.cc | 21 +++--
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/gcc/sched-deps.cc b/gcc/sched-deps.cc
index 005fc0f567e..4d357079a7a 100644
--- a/gcc/sched-deps.cc
+++ b/gcc/sched-deps.cc
@@ -132,7 +132,8 @@ static void dump_ds (FILE *, ds_t);
/* Define flags for dump_dep (). */
/* Dump producer of the dependence. */
-#define DUMP_DEP_PRO (2)
+#define DUMP_DEP_PRO (2) /* Reserve "1" for handling of DUMP_DEP_ALL and
+ DUMP_LISTS_ALL. */
/* Dump consumer of the dependence. */
#define DUMP_DEP_CON (4)
@@ -206,9 +207,6 @@ dump_dep (FILE *dump, dep_t dep, int flags)
fprintf (dump, ">");
}
-/* Default flags for dump_dep (). */
-static int dump_dep_flags = (DUMP_DEP_PRO | DUMP_DEP_CON);
-
/* Dump all fields of DEP to STDERR. */
void
sd_debug_dep (dep_t dep)
@@ -1454,19 +1452,20 @@ sd_delete_dep (sd_iterator_def sd_it)
}
/* Dump size of the lists. */
-#define DUMP_LISTS_SIZE (2)
+#define DUMP_LISTS_SIZE (32) /* (DUMP_DEP_STATUS << 1) */
/* Dump dependencies of the lists. */
-#define DUMP_LISTS_DEPS (4)
+#define DUMP_LISTS_DEPS (64)
/* Dump all information about the lists. */
#define DUMP_LISTS_ALL (DUMP_LISTS_SIZE | DUMP_LISTS_DEPS)
/* Dump deps_lists of INSN specified by TYPES to DUMP.
- FLAGS is a bit mask specifying what information about the lists needs
- to be printed.
+ FLAGS is a bit mask specifying what information about the lists and
+ the individual deps needs to be printed, this is a combination of
+ DUMP_DEP_* and DUMP_LISTS_* flags.
If FLAGS has the very first bit set, then dump all information about
- the lists and propagate this bit into the callee dump functions. */
+ the lists and deps propagate this bit into the callee dump functions. */
static void
dump_lists (FILE *dump, rtx insn, sd_list_types_def types, int flags)
{
@@ -1488,10 +1487,12 @@ dump_lists (FILE *dump, rtx insn, sd_list_types_def
types, int flags)
{
FOR_EACH_DEP (insn, types, sd_it, dep)
{
- dump_dep (dump, dep, dump_dep_flags | all);
+ dump_dep (dump, dep, flags | all);
fprintf (dump, " ");
}
}
+
+ fprintf (dump, "]");
}
/* Dump all information about deps_lists of INSN specified by TYPES
--
2.34.1
[PATCH v3 3/8] Simplify handling of INSN_ and EXPR_LISTs in sched-rgn.cc
This patch simplifies logic behind deps_join(), which will be
important for the upcoming improvements of sched-deps.cc logging.
The only functional change is that when deps_join() is called with
empty state for the 2nd argument, it will not reverse INSN_ and
EXPR_LISTs in the 1st argument. Before this patch the lists were
reversed due to use of concat_*_LIST(). Now, with copy_*_LIST()
used for this case, the lists will remain in the original order.
gcc/ChangeLog:
* lists.cc (copy_EXPR_LIST, concat_EXPR_LIST): New functions.
* rtl.h (copy_EXPR_LIST, concat_EXPR_LIST): Declare.
* sched-rgn.cc (concat_insn_list, concat_expr_list): New helpers.
(concat_insn_mem_list): Simplify.
(deps_join): Update
---
gcc/lists.cc | 30 +++-
gcc/rtl.h| 4 +++-
gcc/sched-rgn.cc | 51 +++-
3 files changed, 61 insertions(+), 24 deletions(-)
diff --git a/gcc/lists.cc b/gcc/lists.cc
index 2cdf37ad533..83e7bf32176 100644
--- a/gcc/lists.cc
+++ b/gcc/lists.cc
@@ -160,6 +160,24 @@ free_INSN_LIST_list (rtx_insn_list **listp)
free_list ((rtx *)listp, _insn_list);
}
+/* Make a copy of the EXPR_LIST list LINK and return it. */
+rtx_expr_list *
+copy_EXPR_LIST (rtx_expr_list *link)
+{
+ rtx_expr_list *new_queue;
+ rtx_expr_list **pqueue = _queue;
+
+ for (; link; link = link->next ())
+{
+ rtx x = link->element ();
+ rtx_expr_list *newlink = alloc_EXPR_LIST (REG_NOTE_KIND (link), x, NULL);
+ *pqueue = newlink;
+ pqueue = (rtx_expr_list **) (newlink, 1);
+}
+ *pqueue = NULL;
+ return new_queue;
+}
+
/* Make a copy of the INSN_LIST list LINK and return it. */
rtx_insn_list *
copy_INSN_LIST (rtx_insn_list *link)
@@ -178,12 +196,22 @@ copy_INSN_LIST (rtx_insn_list *link)
return new_queue;
}
+/* Duplicate the EXPR_LIST elements of COPY and prepend them to OLD. */
+rtx_expr_list *
+concat_EXPR_LIST (rtx_expr_list *copy, rtx_expr_list *old)
+{
+ rtx_expr_list *new_rtx = old;
+ for (; copy; copy = copy->next ())
+new_rtx = alloc_EXPR_LIST (REG_NOTE_KIND (copy), copy->element (),
new_rtx);
+ return new_rtx;
+}
+
/* Duplicate the INSN_LIST elements of COPY and prepend them to OLD. */
rtx_insn_list *
concat_INSN_LIST (rtx_insn_list *copy, rtx_insn_list *old)
{
rtx_insn_list *new_rtx = old;
- for (; copy ; copy = copy->next ())
+ for (; copy; copy = copy->next ())
{
new_rtx = alloc_INSN_LIST (copy->insn (), new_rtx);
PUT_REG_NOTE_KIND (new_rtx, REG_NOTE_KIND (copy));
diff --git a/gcc/rtl.h b/gcc/rtl.h
index e4b6cc0dbb5..7e952d7cbeb 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -3764,10 +3764,12 @@ extern void free_EXPR_LIST_list (rtx_expr_list **);
extern void free_INSN_LIST_list (rtx_insn_list **);
extern void free_EXPR_LIST_node (rtx);
extern void free_INSN_LIST_node (rtx);
+extern rtx_expr_list *alloc_EXPR_LIST (int, rtx, rtx);
extern rtx_insn_list *alloc_INSN_LIST (rtx, rtx);
+extern rtx_expr_list *copy_EXPR_LIST (rtx_expr_list *);
extern rtx_insn_list *copy_INSN_LIST (rtx_insn_list *);
+extern rtx_expr_list *concat_EXPR_LIST (rtx_expr_list *, rtx_expr_list *);
extern rtx_insn_list *concat_INSN_LIST (rtx_insn_list *, rtx_insn_list *);
-extern rtx_expr_list *alloc_EXPR_LIST (int, rtx, rtx);
extern void remove_free_INSN_LIST_elem (rtx_insn *, rtx_insn_list **);
extern rtx remove_list_elem (rtx, rtx *);
extern rtx_insn *remove_free_INSN_LIST_node (rtx_insn_list **);
diff --git a/gcc/sched-rgn.cc b/gcc/sched-rgn.cc
index e5964f54ead..da3ec0458ff 100644
--- a/gcc/sched-rgn.cc
+++ b/gcc/sched-rgn.cc
@@ -2585,25 +2585,32 @@ add_branch_dependences (rtx_insn *head, rtx_insn *tail)
static class deps_desc *bb_deps;
+/* Return a new insn_list with all the elements from the two input lists. */
+static rtx_insn_list *
+concat_insn_list (rtx_insn_list *copy, rtx_insn_list *old)
+{
+ if (!old)
+return copy_INSN_LIST (copy);
+ return concat_INSN_LIST (copy, old);
+}
+
+/* Return a new expr_list with all the elements from the two input lists. */
+static rtx_expr_list *
+concat_expr_list (rtx_expr_list *copy, rtx_expr_list *old)
+{
+ if (!old)
+return copy_EXPR_LIST (copy);
+ return concat_EXPR_LIST (copy, old);
+}
+
static void
concat_insn_mem_list (rtx_insn_list *copy_insns,
rtx_expr_list *copy_mems,
rtx_insn_list **old_insns_p,
rtx_expr_list **old_mems_p)
{
- rtx_insn_list *new_insns = *old_insns_p;
- rtx_expr_list *new_mems = *old_mems_p;
-
- while (copy_insns)
-{
- new_insns = alloc_INSN_LIST (copy_insns->insn (), new_insns);
- new_mems = alloc_EXPR_LIST (VOIDmode, copy_mems->element (), new_mems);
- copy_insns = copy_insns->next ();
- copy_mems = copy_mems->next ();
-}
-
- *old_insns_p = new_insns;
- *old_mems_p = new_mems;
+ *old_insns_p = concat_insn_list (copy_insns, *old_insns_p);
+ *old_mems_p = concat_expr_list
[PATCH v3 8/8] Improve logging of scheduler dependency analysis context
Scheduler dependency analysis uses two main data structures:
1. reg_pending_* data contains effects of INSN on the register file,
which is then incorporated into ...
2. deps_desc object, which contains commulative information about all
instructions processed from deps_desc object's initialization.
This patch adds debug dumping of (2).
Dependency analysis contexts (aka deps_desc objects) are huge, but
each instruction affects only a small amount of data in these objects.
Therefore, it is most useful to dump differential information
compared to the dependency state after previous instruction.
gcc/ChangeLog:
* sched-deps.cc (reset_deps, dump_rtx_insn_list)
(rtx_insn_list_same_p): New helper functions.
(dump_deps_desc_diff): New function to dump dependency information.
(sched_analysis_prev_deps): New static variable.
(sched_analyze_insn): Dump dependency information.
(init_deps_global, finish_deps_global): Handle sched_analysis_prev_deps.
* sched-int.h (struct deps_reg): Update comments.
* sched-rgn.cc (concat_insn_list, concat_expr_list): Update comments.
---
gcc/sched-deps.cc | 197 ++
gcc/sched-int.h | 9 ++-
gcc/sched-rgn.cc | 5 ++
3 files changed, 210 insertions(+), 1 deletion(-)
diff --git a/gcc/sched-deps.cc b/gcc/sched-deps.cc
index f9290c82fd2..edca9927e23 100644
--- a/gcc/sched-deps.cc
+++ b/gcc/sched-deps.cc
@@ -1677,6 +1677,15 @@ delete_all_dependences (rtx_insn *insn)
sd_delete_dep (sd_it);
}
+/* Re-initialize existing dependency context DEPS to be a copy of FROM. */
+static void
+reset_deps (class deps_desc *deps, class deps_desc *from)
+{
+ free_deps (deps);
+ init_deps (deps, false);
+ deps_join (deps, from);
+}
+
/* All insns in a scheduling group except the first should only have
dependencies on the previous insn in the group. So we find the
first instruction in the scheduling group by walking the dependence
@@ -2960,6 +2969,177 @@ dump_reg_pending_data (FILE *file, rtx_insn *insn)
}
}
+/* Dump rtx_insn_list LIST.
+ Consider moving to lists.cc if there are users outside of sched-deps.cc. */
+static void
+dump_rtx_insn_list (FILE *file, rtx_insn_list *list)
+{
+ for (; list; list = list->next ())
+fprintf (file, " %d", INSN_UID (list->insn ()));
+}
+
+/* Return TRUE if lists A and B have same elements in the same order. */
+static bool
+rtx_insn_list_same_p (rtx_insn_list *a, rtx_insn_list *b)
+{
+ for (; a && b; a = a->next (), b = b->next ())
+if (a->insn () != b->insn ())
+ return false;
+
+ if (a || b)
+return false;
+
+ return true;
+}
+
+/* Dump parts of DEPS that are different from PREV.
+ Dumping all information from dependency context produces huge
+ hard-to-analize logs; differential dumping is way more managable. */
+static void
+dump_deps_desc_diff (FILE *file, class deps_desc *deps, class deps_desc *prev)
+{
+ /* Each "paragraph" is a single line of output. */
+
+ /* Note on param_max_pending_list_length:
+ During normal dependency analysis various lists should not exceed this
+ limit. Searching for "!!!" in scheduler logs can point to potential bugs
+ or poorly-handled corner-cases. */
+
+ if (!rtx_insn_list_same_p (deps->pending_read_insns,
+prev->pending_read_insns))
+{
+ fprintf (file, ";; deps pending mem reads length(%d):",
+ deps->pending_read_list_length);
+ if ((deps->pending_read_list_length + deps->pending_write_list_length)
+ >= param_max_pending_list_length)
+ fprintf (file, "%d insns!!!", deps->pending_read_list_length);
+ else
+ dump_rtx_insn_list (file, deps->pending_read_insns);
+ fprintf (file, "\n");
+}
+
+ if (!rtx_insn_list_same_p (deps->pending_write_insns,
+prev->pending_write_insns))
+{
+ fprintf (file, ";; deps pending mem writes length(%d):",
+ deps->pending_write_list_length);
+ if ((deps->pending_read_list_length + deps->pending_write_list_length)
+ >= param_max_pending_list_length)
+ fprintf (file, "%d insns!!!", deps->pending_write_list_length);
+ else
+ dump_rtx_insn_list (file, deps->pending_write_insns);
+ fprintf (file, "\n");
+}
+
+ if (!rtx_insn_list_same_p (deps->pending_jump_insns,
+prev->pending_jump_insns))
+{
+ fprintf (file, ";; deps pending jump length(%d):",
+ deps->pending_flush_length);
+ if (deps->pending_flush_length >= param_max_pending_list_length)
+ fprintf (file, "%d insns!!!", deps->pending_flush_length);
+ else
+ dump_rtx_insn_list (file, deps->pending_jump_insns);
+ fprintf (file, "\n");
+}
+
+ fprintf (file, ";; last");
+ if (!rtx_insn_list_same_p (deps->last_pending_memory_flush,
+prev->last_pending_memory_flush))
+{
+ fprintf
[PATCH v3 2/8] Unify implementations of print_hard_reg_set()
We currently have 3 implementations of print_hard_reg_set()
(all with the same name!) in ira-color.cc, ira-conflicts.cc, and
sel-sched-dump.cc. This patch generalizes implementation in
ira-color.cc, and uses it in all other places. The declaration
is added to hard-reg-set.h.
The motivation for this patch is the [upcoming] need for
print_hard_reg_set() in sched-deps.cc.
gcc/ChangeLog:
* hard-reg-set.h (print_hard_reg_set): Declare.
* ira-color.cc (print_hard_reg_set): Generalize a bit.
(debug_hard_reg_set, print_hard_regs_subforest,)
(setup_allocno_available_regs_num): Update.
* ira-conflicts.cc (print_hard_reg_set): Remove.
(print_allocno_conflicts): Use global print_hard_reg_set().
* sel-sched-dump.cc (print_hard_reg_set): Remove.
(dump_hard_reg_set): Use global print_hard_reg_set().
* sel-sched-dump.h (dump_hard_reg_set): Mark as DEBUG_FUNCTION.
---
gcc/hard-reg-set.h| 3 +++
gcc/ira-color.cc | 17 ++---
gcc/ira-conflicts.cc | 39 ---
gcc/sel-sched-dump.cc | 21 -
gcc/sel-sched-dump.h | 2 +-
5 files changed, 22 insertions(+), 60 deletions(-)
diff --git a/gcc/hard-reg-set.h b/gcc/hard-reg-set.h
index b0bb9bce074..81bca6df0e5 100644
--- a/gcc/hard-reg-set.h
+++ b/gcc/hard-reg-set.h
@@ -524,4 +524,7 @@ call_used_or_fixed_reg_p (unsigned int regno)
}
#endif
+/* ira-color.cc */
+extern void print_hard_reg_set (FILE *, HARD_REG_SET, const char *, bool);
+
#endif /* ! GCC_HARD_REG_SET_H */
diff --git a/gcc/ira-color.cc b/gcc/ira-color.cc
index f2e8ea34152..43564b44933 100644
--- a/gcc/ira-color.cc
+++ b/gcc/ira-color.cc
@@ -482,11 +482,14 @@ first_common_ancestor_node (allocno_hard_regs_node_t
first,
}
/* Print hard reg set SET to F. */
-static void
-print_hard_reg_set (FILE *f, HARD_REG_SET set, bool new_line_p)
+void
+print_hard_reg_set (FILE *f, HARD_REG_SET set,
+ const char *title, bool new_line_p)
{
int i, start, end;
+ if (title)
+fprintf (f, "%s", title);
for (start = end = -1, i = 0; i < FIRST_PSEUDO_REGISTER; i++)
{
bool reg_included = TEST_HARD_REG_BIT (set, i);
@@ -516,7 +519,7 @@ print_hard_reg_set (FILE *f, HARD_REG_SET set, bool
new_line_p)
DEBUG_FUNCTION void
debug_hard_reg_set (HARD_REG_SET set)
{
- print_hard_reg_set (stderr, set, true);
+ print_hard_reg_set (stderr, set, NULL, true);
}
/* Print allocno hard register subforest given by ROOTS and its LEVEL
@@ -534,7 +537,7 @@ print_hard_regs_subforest (FILE *f,
allocno_hard_regs_node_t roots,
for (i = 0; i < level * 2; i++)
fprintf (f, " ");
fprintf (f, "%d:(", node->preorder_num);
- print_hard_reg_set (f, node->hard_regs->set, false);
+ print_hard_reg_set (f, node->hard_regs->set, NULL, false);
fprintf (f, ")@%" PRId64"\n", node->hard_regs->cost);
print_hard_regs_subforest (f, node->first, level + 1);
}
@@ -2982,12 +2985,12 @@ setup_allocno_available_regs_num (ira_allocno_t a)
" Allocno a%dr%d of %s(%d) has %d avail. regs ",
ALLOCNO_NUM (a), ALLOCNO_REGNO (a),
reg_class_names[aclass], ira_class_hard_regs_num[aclass], n);
- print_hard_reg_set (ira_dump_file, data->profitable_hard_regs, false);
+ print_hard_reg_set (ira_dump_file, data->profitable_hard_regs, NULL, false);
fprintf (ira_dump_file, ", %snode: ",
data->profitable_hard_regs == data->hard_regs_node->hard_regs->set
? "" : "^");
print_hard_reg_set (ira_dump_file,
- data->hard_regs_node->hard_regs->set, false);
+ data->hard_regs_node->hard_regs->set, NULL, false);
for (i = 0; i < nwords; i++)
{
ira_object_t obj = ALLOCNO_OBJECT (a, i);
@@ -3000,7 +3003,7 @@ setup_allocno_available_regs_num (ira_allocno_t a)
}
fprintf (ira_dump_file, " (confl regs = ");
print_hard_reg_set (ira_dump_file, OBJECT_TOTAL_CONFLICT_HARD_REGS (obj),
- false);
+ NULL, false);
fprintf (ira_dump_file, ")");
}
fprintf (ira_dump_file, "\n");
diff --git a/gcc/ira-conflicts.cc b/gcc/ira-conflicts.cc
index a4d93c8d734..69806b1a15b 100644
--- a/gcc/ira-conflicts.cc
+++ b/gcc/ira-conflicts.cc
@@ -670,37 +670,6 @@ build_conflicts (void)
-/* Print hard reg set SET with TITLE to FILE. */
-static void
-print_hard_reg_set (FILE *file, const char *title, HARD_REG_SET set)
-{
- int i, start, end;
-
- fputs (title, file);
- for (start = end = -1, i = 0; i < FIRST_PSEUDO_REGISTER; i++)
-{
- bool reg_included = TEST_HARD_REG_BIT (set, i);
-
- if (reg_included)
- {
- if (start == -1)
- start = i;
- end = i;
- }
- if (start >= 0 && (!reg_included || i == FIRST_PSEUDO_REGISTER - 1))
- {
- if (start == end)
- fprintf (file, " %d", start);
- else if (start == end + 1)
-
[PATCH v3 6/8] sched_deps.cc: Simplify initialization of dependency contexts
gcc/ChangeLog:
* sched-deps.cc (init_deps, init_deps_reg_last): Simplify.
(free_deps): Remove useless code.
---
gcc/sched-deps.cc | 13 -
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/gcc/sched-deps.cc b/gcc/sched-deps.cc
index 2a87158ba4b..e0d3c97d935 100644
--- a/gcc/sched-deps.cc
+++ b/gcc/sched-deps.cc
@@ -3927,10 +3927,9 @@ init_deps (class deps_desc *deps, bool lazy_reg_last)
int max_reg = (reload_completed ? FIRST_PSEUDO_REGISTER : max_reg_num ());
deps->max_reg = max_reg;
- if (lazy_reg_last)
-deps->reg_last = NULL;
- else
-deps->reg_last = XCNEWVEC (struct deps_reg, max_reg);
+ deps->reg_last = NULL;
+ if (!lazy_reg_last)
+init_deps_reg_last (deps);
INIT_REG_SET (>reg_last_in_use);
deps->pending_read_insns = 0;
@@ -3961,9 +3960,7 @@ init_deps (class deps_desc *deps, bool lazy_reg_last)
void
init_deps_reg_last (class deps_desc *deps)
{
- gcc_assert (deps && deps->max_reg > 0);
- gcc_assert (deps->reg_last == NULL);
-
+ gcc_assert (deps && deps->max_reg > 0 && deps->reg_last == NULL);
deps->reg_last = XCNEWVEC (struct deps_reg, deps->max_reg);
}
@@ -4013,8 +4010,6 @@ free_deps (class deps_desc *deps)
it at all. */
free (deps->reg_last);
deps->reg_last = NULL;
-
- deps = NULL;
}
/* Remove INSN from dependence contexts DEPS. */
--
2.34.1
[PATCH v3 5/8] Add a bit more logging scheduler's dependency analysis
gcc/ChangeLog:
* sched-deps.cc (sd_add_dep, find_inc): Add logging about
dependency creation.
---
gcc/sched-deps.cc | 30 ++
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/gcc/sched-deps.cc b/gcc/sched-deps.cc
index 4d357079a7a..2a87158ba4b 100644
--- a/gcc/sched-deps.cc
+++ b/gcc/sched-deps.cc
@@ -1342,6 +1342,13 @@ sd_add_dep (dep_t dep, bool resolved_p)
in the bitmap caches of dependency information. */
if (true_dependency_cache != NULL)
set_dependency_caches (dep);
+
+ if (sched_verbose >= 9)
+{
+ fprintf (sched_dump, "created dependency ");
+ dump_dep (sched_dump, dep, 1);
+ fprintf (sched_dump, "\n");
+}
}
/* Add or update backward dependence between INSN and ELEM
@@ -4879,18 +4886,33 @@ find_inc (struct mem_inc_info *mii, bool backwards)
we create. */
gcc_assert (mii->inc_insn == inc_cand);
+ int n_deps_created = 0;
if (backwards)
{
FOR_EACH_DEP (mii->inc_insn, SD_LIST_BACK, sd_it, dep)
- add_dependence_1 (mii->mem_insn, DEP_PRO (dep),
- REG_DEP_TRUE);
+ {
+ add_dependence_1 (mii->mem_insn, DEP_PRO (dep),
+ REG_DEP_TRUE);
+ ++n_deps_created;
+ }
}
else
{
FOR_EACH_DEP (mii->inc_insn, SD_LIST_FORW, sd_it, dep)
- add_dependence_1 (DEP_CON (dep), mii->mem_insn,
- REG_DEP_ANTI);
+ {
+ add_dependence_1 (DEP_CON (dep), mii->mem_insn,
+ REG_DEP_ANTI);
+ ++n_deps_created;
+ }
}
+ if (sched_verbose >= 6)
+ fprintf (sched_dump,
+"created %d deps for mem_insn %d due to "
+"inc_insn %d %s deps\n",
+n_deps_created, INSN_UID (mii->mem_insn),
+INSN_UID (mii->inc_insn),
+backwards ? "backward" : "forward");
+
return true;
}
next:
--
2.34.1
[PATCH v3 0/8] Avoid exponential behavior in scheduler and better logging
Compared to v1 and v2 this is a complete patch series. The debugging/dumping improvements gently touch IRA, RTL lists, and sel-sched bits to avoid re-inventing or copy-pasting the wheel. Bootstrapping and regtesting these patches on aarch64-linux-gnu. OK to merge? Thanks! === This patch series fixes exponential behavior in scheduler's find_modifiable_mems(). This fixes PRs [1] and [2], which are compilation time and memory hogs. The first patch in the series is the actual fix (bootstrapped and regtested on aarch64-linux-gnu), and follow up patches are improvements to scheduler's logging infrastructure, that enabled me to debug this problem. As-is, the scheduler has good logging of the actual scheduling process, but calculation of instruction dependencies has almost no logging. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96388 [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111554 Maxim Kuvyrkov (8): sched-deps.cc (find_modifiable_mems): Avoid exponential behavior Unify implementations of print_hard_reg_set() Simplify handling of INSN_ and EXPR_LISTs in sched-rgn.cc Improve and fix sched-deps.cc: dump_dep() and dump_lists(). Add a bit more logging scheduler's dependency analysis sched_deps.cc: Simplify initialization of dependency contexts Improve logging of register data in scheduler dependency analysis Improve logging of scheduler dependency analysis context gcc/hard-reg-set.h| 3 + gcc/ira-color.cc | 17 +- gcc/ira-conflicts.cc | 39 + gcc/lists.cc | 30 +++- gcc/rtl.h | 4 +- gcc/sched-deps.cc | 399 +++--- gcc/sched-int.h | 9 +- gcc/sched-rgn.cc | 56 +++--- gcc/sel-sched-dump.cc | 21 +-- gcc/sel-sched-dump.h | 2 +- 10 files changed, 467 insertions(+), 113 deletions(-) -- 2.34.1
[PATCH v3 1/8] sched-deps.cc (find_modifiable_mems): Avoid exponential behavior
This patch avoids sched-deps.cc:find_inc() creating exponential number
of dependencies, which become memory and compilation time hogs.
Consider example (simplified from PR96388) ...
===
sp=sp-4 // sp_insnA
mem_insnA1[sp+A1]
...
mem_insnAN[sp+AN]
sp=sp-4 // sp_insnB
mem_insnB1[sp+B1]
...
mem_insnBM[sp+BM]
===
[For simplicity, let's assume find_inc(backwards==true)].
In this example find_modifiable_mems() will arrange for mem_insnA*
to be able to pass sp_insnA, and, while doing this, will create
dependencies between all mem_insnA*s and sp_insnB -- because sp_insnB
is a consumer of sp_insnA. After this sp_insnB will have N new
backward dependencies.
Then find_modifiable_mems() gets to mem_insnB*s and starts to create
N new dependencies for _every_ mem_insnB*. This gets us N*M new
dependencies.
In PR96833's testcase N and M are 10k-15k, which causes RAM usage of
30GB and compilation time of 30 minutes, with sched2 accounting for
95% of both metrics. After this patch the RAM usage is down to 1GB
and compilation time is down to 3-4 minutes, with sched2 no longer
standing out on -ftime-report or memory usage.
gcc/ChangeLog:
PR rtl-optimization/96388
PR rtl-optimization/111554
* sched-deps.cc (find_inc): Avoid exponential behavior.
---
gcc/sched-deps.cc | 48 +++
1 file changed, 44 insertions(+), 4 deletions(-)
diff --git a/gcc/sched-deps.cc b/gcc/sched-deps.cc
index c23218890f3..005fc0f567e 100644
--- a/gcc/sched-deps.cc
+++ b/gcc/sched-deps.cc
@@ -4779,24 +4779,59 @@ parse_add_or_inc (struct mem_inc_info *mii, rtx_insn
*insn, bool before_mem)
/* Once a suitable mem reference has been found and the corresponding data
in MII has been filled in, this function is called to find a suitable
add or inc insn involving the register we found in the memory
- reference. */
+ reference.
+ If successful, this function will create additional dependencies between
+ - mii->inc_insn's producers and mii->mem_insn as a consumer (if backwards)
+ - mii->inc_insn's consumers and mii->mem_insn as a producer (if !backwards).
+*/
static bool
find_inc (struct mem_inc_info *mii, bool backwards)
{
sd_iterator_def sd_it;
dep_t dep;
+ sd_list_types_def mem_deps = backwards ? SD_LIST_HARD_BACK : SD_LIST_FORW;
+ int n_mem_deps = sd_lists_size (mii->mem_insn, mem_deps);
- sd_it = sd_iterator_start (mii->mem_insn,
-backwards ? SD_LIST_HARD_BACK : SD_LIST_FORW);
+ sd_it = sd_iterator_start (mii->mem_insn, mem_deps);
while (sd_iterator_cond (_it, ))
{
dep_node_t node = DEP_LINK_NODE (*sd_it.linkp);
rtx_insn *pro = DEP_PRO (dep);
rtx_insn *con = DEP_CON (dep);
- rtx_insn *inc_cand = backwards ? pro : con;
+ rtx_insn *inc_cand;
+ int n_inc_deps;
+
if (DEP_NONREG (dep) || DEP_MULTIPLE (dep))
goto next;
+
+ if (backwards)
+ {
+ inc_cand = pro;
+ n_inc_deps = sd_lists_size (inc_cand, SD_LIST_BACK);
+ }
+ else
+ {
+ inc_cand = con;
+ n_inc_deps = sd_lists_size (inc_cand, SD_LIST_FORW);
+ }
+
+ /* In the FOR_EACH_DEP loop below we will create additional n_inc_deps
+for mem_insn. This by itself is not a problem, since each mem_insn
+will have only a few inc_insns associated with it. However, if
+we consider that a single inc_insn may have a lot of mem_insns, AND,
+on top of that, a few other inc_insns associated with it --
+those _other inc_insns_ will get (n_mem_deps * number of MEM insns)
+dependencies created for them. This may cause an exponential
+growth of memory usage and scheduling time.
+See PR96388 for details.
+We [heuristically] use n_inc_deps as a proxy for the number of MEM
+insns, and drop opportunities for breaking modifiable_mem dependencies
+when dependency lists grow beyond reasonable size. */
+ if (n_mem_deps * n_inc_deps
+ >= param_max_pending_list_length * param_max_pending_list_length)
+ goto next;
+
if (parse_add_or_inc (mii, inc_cand, backwards))
{
struct dep_replacement *desc;
@@ -4838,6 +4873,11 @@ find_inc (struct mem_inc_info *mii, bool backwards)
desc->insn = mii->mem_insn;
move_dep_link (DEP_NODE_BACK (node), INSN_HARD_BACK_DEPS (con),
INSN_SPEC_BACK_DEPS (con));
+
+ /* Make sure that n_inc_deps above is consistent with dependencies
+we create. */
+ gcc_assert (mii->inc_insn == inc_cand);
+
if (backwards)
{
FOR_EACH_DEP (mii->inc_insn, SD_LIST_BACK, sd_it, dep)
--
2.34.1
Re: [PATCH 1/1] sched-deps.cc (find_modifiable_mems): Avoid exponential behavior
> On Nov 20, 2023, at 20:30, Alexander Monakov wrote: > > > On Mon, 20 Nov 2023, Maxim Kuvyrkov wrote: > >>> On Nov 20, 2023, at 17:52, Alexander Monakov wrote: >>> >>> >>> On Mon, 20 Nov 2023, Maxim Kuvyrkov wrote: >>> >>>> This patch avoids sched-deps.cc:find_inc() creating exponential number >>>> of dependencies, which become memory and compilation time hogs. >>>> Consider example (simplified from PR96388) ... >>>> === >>>> sp=sp-4 // sp_insnA >>>> mem_insnA1[sp+A1] >>>> ... >>>> mem_insnAN[sp+AN] >>>> sp=sp-4 // sp_insnB >>>> mem_insnB1[sp+B1] >>>> ... >>>> mem_insnBM[sp+BM] >>>> === >>>> ... in this example find_modifiable_mems() will arrange for mem_insnA* >>>> to be able to pass sp_insnA, and, while doing this, will create >>>> dependencies between all mem_insnA*s and sp_insnB -- because sp_insnB >>>> is a consumer of sp_insnA. After this sp_insnB will have N new >>>> backward dependencies. >>>> Then find_modifiable_mems() gets to mem_insnB*s and starts to create >>>> N new dependencies for _every_ mem_insnB*. This gets us N*M new >>>> dependencies. >> >> [For avoidance of doubt, below discussion is about the general implementation >> of find_modifiable_mems() and not about the patch.] > > I was saying the commit message is hard to read (unless it's just me). > >>> It's a bit hard to read this without knowing which value of 'backwards' >>> is assumed. Oh, sorry, I misunderstood your comment. In the above example I want to describe outcome that current code generates, without going into details about exactly how it does it. I'm not sure how to make it more readable, and would appreciate suggestions. >>> >>> Say 'backwards' is true and we are inspecting producer sp_insnB of >>> mem_insnB1. >>> This is a true dependency. We know we can break it by adjusting B1 by -4, >>> but >>> we need to be careful not to move such modified mem_insnB1 above sp_insnA, >>> so >>> we need to iterate over *incoming true dependencies* of sp_insnB and add >>> them. >>> >>> But the code seems to be iterating over *all incoming dependencies*, so it >>> will e.g. take anti-dependency mem_insnA1 -> sp_insnB and create a true >>> dependency mem_insnA1 -> mem_insnB1'. This seems utterly inefficient, if my >>> understanding is correct. >> >> Yeap, your understanding is correct. However, this is what >> find_modifiable_mems() has to do to avoid complicated analysis of >> second-level >> dependencies. > > What is the reason it cannot simply skip anti-dependencies in the > 'if (backwards)' loop, and true dependencies in the 'else' loop? I /think/, this should be possible. However, rather than improving current implementation my preference is to rework it by integrating with the main dependency analysis. This should provide both faster and more precise dependency analysis, which would generate breakable addr/mem dependencies. Thanks, -- Maxim Kuvyrkov https://www.linaro.org
Re: [PATCH 1/1] sched-deps.cc (find_modifiable_mems): Avoid exponential behavior
> On Nov 20, 2023, at 17:45, Richard Biener wrote:
>
> On Mon, Nov 20, 2023 at 2:42 PM Maxim Kuvyrkov
> wrote:
>>
>>> On Nov 20, 2023, at 17:09, Richard Biener
>>> wrote:
>>>
>>> On Mon, Nov 20, 2023 at 1:08 PM Maxim Kuvyrkov
>>> wrote:
>>>>
>>>> This patch avoids sched-deps.cc:find_inc() creating exponential number
>>>> of dependencies, which become memory and compilation time hogs.
>>>> Consider example (simplified from PR96388) ...
>>>> ===
>>>> sp=sp-4 // sp_insnA
>>>> mem_insnA1[sp+A1]
>>>> ...
>>>> mem_insnAN[sp+AN]
>>>> sp=sp-4 // sp_insnB
>>>> mem_insnB1[sp+B1]
>>>> ...
>>>> mem_insnBM[sp+BM]
>>>> ===
>>>> ... in this example find_modifiable_mems() will arrange for mem_insnA*
>>>> to be able to pass sp_insnA, and, while doing this, will create
>>>> dependencies between all mem_insnA*s and sp_insnB -- because sp_insnB
>>>> is a consumer of sp_insnA. After this sp_insnB will have N new
>>>> backward dependencies.
>>>> Then find_modifiable_mems() gets to mem_insnB*s and starts to create
>>>> N new dependencies for _every_ mem_insnB*. This gets us N*M new
>>>> dependencies.
>>>>
>>>> In PR96833's testcase N and M are 10k-15k, which causes RAM usage of
>>>> 30GB and compilation time of 30 minutes, with sched2 accounting for
>>>> 95% of both metrics. After this patch the RAM usage is down to 1GB
>>>> and compilation time is down to 3-4 minutes, with sched2 no longer
>>>> standing out on -ftime-report or memory usage.
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>> PR rtl-optimization/96388
>>>> PR rtl-optimization/111554
>>>> * sched-deps.cc (find_inc): Avoid exponential behavior.
>>>> ---
>>>> gcc/sched-deps.cc | 45 +
>>>> 1 file changed, 41 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/gcc/sched-deps.cc b/gcc/sched-deps.cc
>>>> index c23218890f3..397bb9fd462 100644
>>>> --- a/gcc/sched-deps.cc
>>>> +++ b/gcc/sched-deps.cc
>>>> @@ -4779,24 +4779,59 @@ parse_add_or_inc (struct mem_inc_info *mii,
>>>> rtx_insn *insn, bool before_mem)
>>>> /* Once a suitable mem reference has been found and the corresponding data
>>>> in MII has been filled in, this function is called to find a suitable
>>>> add or inc insn involving the register we found in the memory
>>>> - reference. */
>>>> + reference.
>>>> + If successful, this function will create additional dependencies
>>>> between
>>>> + - mii->inc_insn's producers and mii->mem_insn as a consumer (if
>>>> backwards)
>>>> + - mii->inc_insn's consumers and mii->mem_insn as a producer (if
>>>> !backwards).
>>>> +*/
>>>>
>>>> static bool
>>>> find_inc (struct mem_inc_info *mii, bool backwards)
>>>> {
>>>> sd_iterator_def sd_it;
>>>> dep_t dep;
>>>> + sd_list_types_def mem_deps = backwards ? SD_LIST_HARD_BACK :
>>>> SD_LIST_FORW;
>>>> + int n_mem_deps = sd_lists_size (mii->mem_insn, mem_deps);
>>>>
>>>> - sd_it = sd_iterator_start (mii->mem_insn,
>>>> -backwards ? SD_LIST_HARD_BACK : SD_LIST_FORW);
>>>> + sd_it = sd_iterator_start (mii->mem_insn, mem_deps);
>>>> while (sd_iterator_cond (_it, ))
>>>>{
>>>> dep_node_t node = DEP_LINK_NODE (*sd_it.linkp);
>>>> rtx_insn *pro = DEP_PRO (dep);
>>>> rtx_insn *con = DEP_CON (dep);
>>>> - rtx_insn *inc_cand = backwards ? pro : con;
>>>> + rtx_insn *inc_cand;
>>>> + int n_inc_deps;
>>>> +
>>>> + if (backwards)
>>>> + {
>>>> + inc_cand = pro;
>>>> + n_inc_deps = sd_lists_size (inc_cand, SD_LIST_BACK);
>>>> + }
>>>> + else
>>>> + {
>>>> + inc_cand = con;
>>>> + n_inc_deps = sd_lists_size (inc_cand, SD_LIST_FORW);
>>>> + }
>>>> +
>>>> + /* In the FOR_EACH_DEP loop below we will create additional
>>>> n_inc_deps
>>>> +
[PATCH v2] sched-deps.cc (find_modifiable_mems): Avoid exponential behavior
This patch avoids sched-deps.cc:find_inc() creating exponential number
of dependencies, which become memory and compilation time hogs.
Consider example (simplified from PR96388) ...
===
sp=sp-4 // sp_insnA
mem_insnA1[sp+A1]
...
mem_insnAN[sp+AN]
sp=sp-4 // sp_insnB
mem_insnB1[sp+B1]
...
mem_insnBM[sp+BM]
===
... in this example find_modifiable_mems() will arrange for mem_insnA*
to be able to pass sp_insnA, and, while doing this, will create
dependencies between all mem_insnA*s and sp_insnB -- because sp_insnB
is a consumer of sp_insnA. After this sp_insnB will have N new
backward dependencies.
Then find_modifiable_mems() gets to mem_insnB*s and starts to create
N new dependencies for _every_ mem_insnB*. This gets us N*M new
dependencies.
In PR96833's testcase N and M are 10k-15k, which causes RAM usage of
30GB and compilation time of 30 minutes, with sched2 accounting for
95% of both metrics. After this patch the RAM usage is down to 1GB
and compilation time is down to 3-4 minutes, with sched2 no longer
standing out on -ftime-report or memory usage.
gcc/ChangeLog:
PR rtl-optimization/96388
PR rtl-optimization/111554
* sched-deps.cc (find_inc): Avoid exponential behavior.
---
gcc/sched-deps.cc | 45 +
1 file changed, 41 insertions(+), 4 deletions(-)
diff --git a/gcc/sched-deps.cc b/gcc/sched-deps.cc
index c23218890f3..a286ff319e2 100644
--- a/gcc/sched-deps.cc
+++ b/gcc/sched-deps.cc
@@ -4779,24 +4779,59 @@ parse_add_or_inc (struct mem_inc_info *mii, rtx_insn
*insn, bool before_mem)
/* Once a suitable mem reference has been found and the corresponding data
in MII has been filled in, this function is called to find a suitable
add or inc insn involving the register we found in the memory
- reference. */
+ reference.
+ If successful, this function will create additional dependencies between
+ - mii->inc_insn's producers and mii->mem_insn as a consumer (if backwards)
+ - mii->inc_insn's consumers and mii->mem_insn as a producer (if !backwards).
+*/
static bool
find_inc (struct mem_inc_info *mii, bool backwards)
{
sd_iterator_def sd_it;
dep_t dep;
+ sd_list_types_def mem_deps = backwards ? SD_LIST_HARD_BACK : SD_LIST_FORW;
+ int n_mem_deps = sd_lists_size (mii->mem_insn, mem_deps);
- sd_it = sd_iterator_start (mii->mem_insn,
-backwards ? SD_LIST_HARD_BACK : SD_LIST_FORW);
+ sd_it = sd_iterator_start (mii->mem_insn, mem_deps);
while (sd_iterator_cond (_it, ))
{
dep_node_t node = DEP_LINK_NODE (*sd_it.linkp);
rtx_insn *pro = DEP_PRO (dep);
rtx_insn *con = DEP_CON (dep);
- rtx_insn *inc_cand = backwards ? pro : con;
+ rtx_insn *inc_cand;
+ int n_inc_deps;
+
if (DEP_NONREG (dep) || DEP_MULTIPLE (dep))
goto next;
+
+ if (backwards)
+ {
+ inc_cand = pro;
+ n_inc_deps = sd_lists_size (inc_cand, SD_LIST_BACK);
+ }
+ else
+ {
+ inc_cand = con;
+ n_inc_deps = sd_lists_size (inc_cand, SD_LIST_FORW);
+ }
+
+ /* In the FOR_EACH_DEP loop below we will create additional n_inc_deps
+for mem_insn. This by itself is not a problem, since each mem_insn
+will have only a few inc_insns associated with it. However, if
+we consider that a single inc_insn may have a lot of mem_insns, AND,
+on top of that, a few other inc_insns associated with it --
+those _other inc_insns_ will get (n_mem_deps * number of MEM insns)
+dependencies created for them. This may cause an exponential
+growth of memory usage and scheduling time.
+See PR96388 for details.
+We [heuristically] use n_inc_deps as a proxy for the number of MEM
+insns, and drop opportunities for breaking modifiable_mem dependencies
+when dependency lists grow beyond reasonable size. */
+ if (n_mem_deps * n_inc_deps
+ >= param_max_pending_list_length * param_max_pending_list_length)
+ goto next;
+
if (parse_add_or_inc (mii, inc_cand, backwards))
{
struct dep_replacement *desc;
@@ -4838,6 +4873,8 @@ find_inc (struct mem_inc_info *mii, bool backwards)
desc->insn = mii->mem_insn;
move_dep_link (DEP_NODE_BACK (node), INSN_HARD_BACK_DEPS (con),
INSN_SPEC_BACK_DEPS (con));
+
+ gcc_assert (mii->inc_insn == inc_cand);
if (backwards)
{
FOR_EACH_DEP (mii->inc_insn, SD_LIST_BACK, sd_it, dep)
--
2.34.1
Re: [PATCH 1/1] sched-deps.cc (find_modifiable_mems): Avoid exponential behavior
> On Nov 20, 2023, at 17:52, Alexander Monakov wrote: > > > On Mon, 20 Nov 2023, Maxim Kuvyrkov wrote: > >> This patch avoids sched-deps.cc:find_inc() creating exponential number >> of dependencies, which become memory and compilation time hogs. >> Consider example (simplified from PR96388) ... >> === >> sp=sp-4 // sp_insnA >> mem_insnA1[sp+A1] >> ... >> mem_insnAN[sp+AN] >> sp=sp-4 // sp_insnB >> mem_insnB1[sp+B1] >> ... >> mem_insnBM[sp+BM] >> === >> ... in this example find_modifiable_mems() will arrange for mem_insnA* >> to be able to pass sp_insnA, and, while doing this, will create >> dependencies between all mem_insnA*s and sp_insnB -- because sp_insnB >> is a consumer of sp_insnA. After this sp_insnB will have N new >> backward dependencies. >> Then find_modifiable_mems() gets to mem_insnB*s and starts to create >> N new dependencies for _every_ mem_insnB*. This gets us N*M new >> dependencies. [For avoidance of doubt, below discussion is about the general implementation of find_modifiable_mems() and not about the patch.] > > It's a bit hard to read this without knowing which value of 'backwards' > is assumed. > > Say 'backwards' is true and we are inspecting producer sp_insnB of mem_insnB1. > This is a true dependency. We know we can break it by adjusting B1 by -4, but > we need to be careful not to move such modified mem_insnB1 above sp_insnA, so > we need to iterate over *incoming true dependencies* of sp_insnB and add them. > > But the code seems to be iterating over *all incoming dependencies*, so it > will e.g. take anti-dependency mem_insnA1 -> sp_insnB and create a true > dependency mem_insnA1 -> mem_insnB1'. This seems utterly inefficient, if my > understanding is correct. Yeap, your understanding is correct. However, this is what find_modifiable_mems() has to do to avoid complicated analysis of second-level dependencies. I think, the optimization that find_modifiable_mems() does can be implemented as part of main dependency analysis, and I'm going to discuss that with Bernd. I might be missing something, but it seems that instruction transformations that find_modifiable_mems() is doing are simpler than what ia64 speculation is doing. So, I think, we should be able to leverage speculation infrastructure, rather than doing this optimization "on the side" of normal scheduling. -- Maxim Kuvyrkov https://www.linaro.org
Re: [PATCH 1/1] sched-deps.cc (find_modifiable_mems): Avoid exponential behavior
> On Nov 20, 2023, at 17:09, Richard Biener wrote:
>
> On Mon, Nov 20, 2023 at 1:08 PM Maxim Kuvyrkov
> wrote:
>>
>> This patch avoids sched-deps.cc:find_inc() creating exponential number
>> of dependencies, which become memory and compilation time hogs.
>> Consider example (simplified from PR96388) ...
>> ===
>> sp=sp-4 // sp_insnA
>> mem_insnA1[sp+A1]
>> ...
>> mem_insnAN[sp+AN]
>> sp=sp-4 // sp_insnB
>> mem_insnB1[sp+B1]
>> ...
>> mem_insnBM[sp+BM]
>> ===
>> ... in this example find_modifiable_mems() will arrange for mem_insnA*
>> to be able to pass sp_insnA, and, while doing this, will create
>> dependencies between all mem_insnA*s and sp_insnB -- because sp_insnB
>> is a consumer of sp_insnA. After this sp_insnB will have N new
>> backward dependencies.
>> Then find_modifiable_mems() gets to mem_insnB*s and starts to create
>> N new dependencies for _every_ mem_insnB*. This gets us N*M new
>> dependencies.
>>
>> In PR96833's testcase N and M are 10k-15k, which causes RAM usage of
>> 30GB and compilation time of 30 minutes, with sched2 accounting for
>> 95% of both metrics. After this patch the RAM usage is down to 1GB
>> and compilation time is down to 3-4 minutes, with sched2 no longer
>> standing out on -ftime-report or memory usage.
>>
>> gcc/ChangeLog:
>>
>>PR rtl-optimization/96388
>>PR rtl-optimization/111554
>>* sched-deps.cc (find_inc): Avoid exponential behavior.
>> ---
>> gcc/sched-deps.cc | 45 +
>> 1 file changed, 41 insertions(+), 4 deletions(-)
>>
>> diff --git a/gcc/sched-deps.cc b/gcc/sched-deps.cc
>> index c23218890f3..397bb9fd462 100644
>> --- a/gcc/sched-deps.cc
>> +++ b/gcc/sched-deps.cc
>> @@ -4779,24 +4779,59 @@ parse_add_or_inc (struct mem_inc_info *mii, rtx_insn
>> *insn, bool before_mem)
>> /* Once a suitable mem reference has been found and the corresponding data
>>in MII has been filled in, this function is called to find a suitable
>>add or inc insn involving the register we found in the memory
>> - reference. */
>> + reference.
>> + If successful, this function will create additional dependencies between
>> + - mii->inc_insn's producers and mii->mem_insn as a consumer (if
>> backwards)
>> + - mii->inc_insn's consumers and mii->mem_insn as a producer (if
>> !backwards).
>> +*/
>>
>> static bool
>> find_inc (struct mem_inc_info *mii, bool backwards)
>> {
>> sd_iterator_def sd_it;
>> dep_t dep;
>> + sd_list_types_def mem_deps = backwards ? SD_LIST_HARD_BACK : SD_LIST_FORW;
>> + int n_mem_deps = sd_lists_size (mii->mem_insn, mem_deps);
>>
>> - sd_it = sd_iterator_start (mii->mem_insn,
>> -backwards ? SD_LIST_HARD_BACK : SD_LIST_FORW);
>> + sd_it = sd_iterator_start (mii->mem_insn, mem_deps);
>> while (sd_iterator_cond (_it, ))
>> {
>> dep_node_t node = DEP_LINK_NODE (*sd_it.linkp);
>> rtx_insn *pro = DEP_PRO (dep);
>> rtx_insn *con = DEP_CON (dep);
>> - rtx_insn *inc_cand = backwards ? pro : con;
>> + rtx_insn *inc_cand;
>> + int n_inc_deps;
>> +
>> + if (backwards)
>> + {
>> + inc_cand = pro;
>> + n_inc_deps = sd_lists_size (inc_cand, SD_LIST_BACK);
>> + }
>> + else
>> + {
>> + inc_cand = con;
>> + n_inc_deps = sd_lists_size (inc_cand, SD_LIST_FORW);
>> + }
>> +
>> + /* In the FOR_EACH_DEP loop below we will create additional n_inc_deps
>> +for mem_insn. This by itself is not a problem, since each mem_insn
>> +will have only a few inc_insns associated with it. However, if
>> +we consider that a single inc_insn may have a lot of mem_insns, AND,
>> +on top of that, a few other inc_insns associated with it --
>> +those _other inc_insns_ will get (n_mem_deps * number of MEM insns)
>> +dependencies created for them. This may cause an exponential
>> +growth of memory usage and scheduling time.
>> +See PR96388 for details.
>> +We [heuristically] use n_inc_deps as a proxy for the number of MEM
>> +insns, and drop opportunities for breaking modifiable_mem
>> dependencies
>> +when dependency lists grow beyond reasonable size. */
>> + if (n_mem_deps * n_inc_deps
>> + >= param_max_pending_list_le
[PATCH 1/1] sched-deps.cc (find_modifiable_mems): Avoid exponential behavior
This patch avoids sched-deps.cc:find_inc() creating exponential number
of dependencies, which become memory and compilation time hogs.
Consider example (simplified from PR96388) ...
===
sp=sp-4 // sp_insnA
mem_insnA1[sp+A1]
...
mem_insnAN[sp+AN]
sp=sp-4 // sp_insnB
mem_insnB1[sp+B1]
...
mem_insnBM[sp+BM]
===
... in this example find_modifiable_mems() will arrange for mem_insnA*
to be able to pass sp_insnA, and, while doing this, will create
dependencies between all mem_insnA*s and sp_insnB -- because sp_insnB
is a consumer of sp_insnA. After this sp_insnB will have N new
backward dependencies.
Then find_modifiable_mems() gets to mem_insnB*s and starts to create
N new dependencies for _every_ mem_insnB*. This gets us N*M new
dependencies.
In PR96833's testcase N and M are 10k-15k, which causes RAM usage of
30GB and compilation time of 30 minutes, with sched2 accounting for
95% of both metrics. After this patch the RAM usage is down to 1GB
and compilation time is down to 3-4 minutes, with sched2 no longer
standing out on -ftime-report or memory usage.
gcc/ChangeLog:
PR rtl-optimization/96388
PR rtl-optimization/111554
* sched-deps.cc (find_inc): Avoid exponential behavior.
---
gcc/sched-deps.cc | 45 +
1 file changed, 41 insertions(+), 4 deletions(-)
diff --git a/gcc/sched-deps.cc b/gcc/sched-deps.cc
index c23218890f3..397bb9fd462 100644
--- a/gcc/sched-deps.cc
+++ b/gcc/sched-deps.cc
@@ -4779,24 +4779,59 @@ parse_add_or_inc (struct mem_inc_info *mii, rtx_insn
*insn, bool before_mem)
/* Once a suitable mem reference has been found and the corresponding data
in MII has been filled in, this function is called to find a suitable
add or inc insn involving the register we found in the memory
- reference. */
+ reference.
+ If successful, this function will create additional dependencies between
+ - mii->inc_insn's producers and mii->mem_insn as a consumer (if backwards)
+ - mii->inc_insn's consumers and mii->mem_insn as a producer (if !backwards).
+*/
static bool
find_inc (struct mem_inc_info *mii, bool backwards)
{
sd_iterator_def sd_it;
dep_t dep;
+ sd_list_types_def mem_deps = backwards ? SD_LIST_HARD_BACK : SD_LIST_FORW;
+ int n_mem_deps = sd_lists_size (mii->mem_insn, mem_deps);
- sd_it = sd_iterator_start (mii->mem_insn,
-backwards ? SD_LIST_HARD_BACK : SD_LIST_FORW);
+ sd_it = sd_iterator_start (mii->mem_insn, mem_deps);
while (sd_iterator_cond (_it, ))
{
dep_node_t node = DEP_LINK_NODE (*sd_it.linkp);
rtx_insn *pro = DEP_PRO (dep);
rtx_insn *con = DEP_CON (dep);
- rtx_insn *inc_cand = backwards ? pro : con;
+ rtx_insn *inc_cand;
+ int n_inc_deps;
+
+ if (backwards)
+ {
+ inc_cand = pro;
+ n_inc_deps = sd_lists_size (inc_cand, SD_LIST_BACK);
+ }
+ else
+ {
+ inc_cand = con;
+ n_inc_deps = sd_lists_size (inc_cand, SD_LIST_FORW);
+ }
+
+ /* In the FOR_EACH_DEP loop below we will create additional n_inc_deps
+for mem_insn. This by itself is not a problem, since each mem_insn
+will have only a few inc_insns associated with it. However, if
+we consider that a single inc_insn may have a lot of mem_insns, AND,
+on top of that, a few other inc_insns associated with it --
+those _other inc_insns_ will get (n_mem_deps * number of MEM insns)
+dependencies created for them. This may cause an exponential
+growth of memory usage and scheduling time.
+See PR96388 for details.
+We [heuristically] use n_inc_deps as a proxy for the number of MEM
+insns, and drop opportunities for breaking modifiable_mem dependencies
+when dependency lists grow beyond reasonable size. */
+ if (n_mem_deps * n_inc_deps
+ >= param_max_pending_list_length * param_max_pending_list_length)
+ goto next;
+
if (DEP_NONREG (dep) || DEP_MULTIPLE (dep))
goto next;
+
if (parse_add_or_inc (mii, inc_cand, backwards))
{
struct dep_replacement *desc;
@@ -4838,6 +4873,8 @@ find_inc (struct mem_inc_info *mii, bool backwards)
desc->insn = mii->mem_insn;
move_dep_link (DEP_NODE_BACK (node), INSN_HARD_BACK_DEPS (con),
INSN_SPEC_BACK_DEPS (con));
+
+ gcc_assert (mii->inc_insn == inc_cand);
if (backwards)
{
FOR_EACH_DEP (mii->inc_insn, SD_LIST_BACK, sd_it, dep)
--
2.34.1
[PATCH 0/1] Avoid exponential behavior in scheduler and better logging
Hi, This patch series fixes exponential behavior in scheduler's find_modifiable_mems(). This fixes PRs [1] and [2], which are compilation time and memory hogs. The first patch in the series is the actual fix (bootstrapped and regtested on aarch64-linux-gnu), and follow up patches will be improvements to scheduler's logging infrastructure, that enabled me to debug this problem. As-is, the scheduler has good logging of the actual scheduling process, but calculation of instruction dependencies has almost no logging. Please don't delay review of the PRs fix to wait for logging patches to be published, as it'll take me another week to clean them up. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96388 [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=111554 Maxim Kuvyrkov (1): sched-deps.cc (find_modifiable_mems): Avoid exponential behavior gcc/sched-deps.cc | 45 + 1 file changed, 41 insertions(+), 4 deletions(-) -- 2.34.1
Re: PING^1 [PATCH v3] sched: Change no_real_insns_p to no_real_nondebug_insns_p [PR108273]
Hi Kewen,
Below are my comments. I don't want to override Alexander's review, and if the
patch looks good to him, it's fine to ignore my concerns.
My main concern is that this adds a new entity -- forceful skipping of
DEBUG_INSN-only basic blocks -- to the scheduler for a somewhat minor change in
behavior. Unlike NOTEs and LABELs, DEBUG_INSNs are INSNS, and there is already
quite a bit of logic in the scheduler to skip them _as part of normal
operation_.
Would you please consider 2 ideas below.
#1:
After a brief look, I'm guessing this part is causing the problem:
haifa-sched.cc <http://haifa-sched.cc/>:schedule_block():
=== [1]
/* Loop until all the insns in BB are scheduled. */
while ((*current_sched_info->schedule_more_p) ())
{
perform_replacements_new_cycle ();
do
{
start_clock_var = clock_var;
clock_var++;
advance_one_cycle ();
===
and then in the nested loop we have
=== [2]
/* We don't want md sched reorder to even see debug isns, so put
them out right away. */
if (ready.n_ready && DEBUG_INSN_P (ready_element (, 0))
&& (*current_sched_info->schedule_more_p) ())
{
while (ready.n_ready && DEBUG_INSN_P (ready_element (, 0)))
{
rtx_insn *insn = ready_remove_first ();
gcc_assert (DEBUG_INSN_P (insn));
(*current_sched_info->begin_schedule_ready) (insn);
scheduled_insns.safe_push (insn);
last_scheduled_insn = insn;
advance = schedule_insn (insn);
gcc_assert (advance == 0);
if (ready.n_ready > 0)
ready_sort ();
}
}
===
. At the [1] point we already have sorted ready list, and I don't see any
blockers to doing [2] before calling advance_one_cycle().
#2
Another approach, which might be even easier, is to save the state of DFA
before the initial advance_one_cycle(), and then restore it if no real insns
have been scheduled.
Kind regards,
--
Maxim Kuvyrkov
https://www.linaro.org
> On Nov 8, 2023, at 06:49, Kewen.Lin wrote:
>
> Hi,
>
> Gentle ping this:
>
> https://gcc.gnu.org/pipermail/gcc-patches/2023-October/634201.html
>
> BR,
> Kewen
>
> on 2023/10/25 10:45, Kewen.Lin wrote:
>> Hi,
>>
>> This is almost a repost for v2 which was posted at[1] in March
>> excepting for:
>> 1) rebased from r14-4810 which is relatively up-to-date,
>> some conflicts on "int to bool" return type change have
>> been resolved;
>> 2) adjust commit log a bit;
>> 3) fix misspelled "articial" with "artificial" somewhere;
>>
>> --
>> *v2 comments*:
>>
>> By addressing Alexander's comments, against v1 this
>> patch v2 mainly:
>>
>> - Rename no_real_insns_p to no_real_nondebug_insns_p;
>> - Introduce enum rgn_bb_deps_free_action for three
>>kinds of actions to free deps;
>> - Change function free_deps_for_bb_no_real_insns_p to
>>resolve_forw_deps which only focuses on forward deps;
>> - Extend the handlings to cover dbg-cnt sched_block,
>>add one test case for it;
>> - Move free_trg_info call in schedule_region to an
>>appropriate place.
>>
>> One thing I'm not sure about is the change in function
>> sched_rgn_local_finish, currently the invocation to
>> sched_rgn_local_free is guarded with !sel_sched_p (),
>> so I just follow it, but the initialization of those
>> structures (in sched_rgn_local_init) isn't guarded
>> with !sel_sched_p (), it looks odd.
>>
>> --
>>
>> As PR108273 shows, when there is one block which only has
>> NOTE_P and LABEL_P insns at non-debug mode while has some
>> extra DEBUG_INSN_P insns at debug mode, after scheduling
>> it, the DFA states would be different between debug mode
>> and non-debug mode. Since at non-debug mode, the block
>> meets no_real_insns_p, it gets skipped; while at debug
>> mode, it gets scheduled, even it only has NOTE_P, LABEL_P
>> and DEBUG_INSN_P, the call of function advance_one_cycle
>> will change the DFA state. PR108519 also shows this issue
>> can be exposed by some scheduler changes.
>>
>> This patch is to change function no_real_insns_p to
>> function no_real_nondebug_insns_p by taking debug insn into
>> account, which make us not try to schedule for the block
>> having only NOTE_P, LABEL_P and DEBUG_INSN_P insns,
>> resulting in consistent DFA states between non-debug and
>> debug mode.
>>
>> Changing no_real_insns_p to no_real_nondebug_insns_p caused
[PATCH v2] Format gotools.sum closer to what DejaGnu does
The only difference compared to v1 is using vanilla automake 1.15.1
to regenerate Makefile.in.
I'll merge this as obvious if no-one objects in a day.
===
... to restore compatability with validate_failures.py .
The testsuite script validate_failures.py expects
"Running ..." to extract values,
and gotools.sum provided "Running ".
Note that libgo.sum, which also uses Makefile logic to generate
DejaGnu-like output, already has "..." suffix.
gotools/ChangeLog:
* Makefile.am: Update "Running ..." output
* Makefile.in: Regenerate.
Signed-off-by: Maxim Kuvyrkov
---
gotools/Makefile.am | 4 ++--
gotools/Makefile.in | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/gotools/Makefile.am b/gotools/Makefile.am
index 7b5302990f8..d2376b9c25b 100644
--- a/gotools/Makefile.am
+++ b/gotools/Makefile.am
@@ -332,8 +332,8 @@ check: check-head check-go-tool check-runtime
check-cgo-test check-carchive-test
@cp gotools.sum gotools.log
@for file in cmd_go-testlog runtime-testlog cgo-testlog
carchive-testlog cmd_vet-testlog embed-testlog; do \
testname=`echo $${file} | sed -e 's/-testlog//' -e 's|_|/|'`; \
- echo "Running $${testname}" >> gotools.sum; \
- echo "Running $${testname}" >> gotools.log; \
+ echo "Running $${testname} ..." >> gotools.sum; \
+ echo "Running $${testname} ..." >> gotools.log; \
sed -e 's/^--- \(.*\) ([^)]*)$$/\1/' < $${file} >> gotools.log; \
grep '^--- ' $${file} | sed -e 's/^--- \(.*\) ([^)]*)$$/\1/' -e
's/SKIP/UNTESTED/' | sort -k 2 >> gotools.sum; \
done
diff --git a/gotools/Makefile.in b/gotools/Makefile.in
index 2783b91ef4b..36c2ec2abd3 100644
--- a/gotools/Makefile.in
+++ b/gotools/Makefile.in
@@ -1003,8 +1003,8 @@ mostlyclean-local:
@NATIVE_TRUE@ @cp gotools.sum gotools.log
@NATIVE_TRUE@ @for file in cmd_go-testlog runtime-testlog cgo-testlog
carchive-testlog cmd_vet-testlog embed-testlog; do \
@NATIVE_TRUE@testname=`echo $${file} | sed -e 's/-testlog//' -e 's|_|/|'`;
\
-@NATIVE_TRUE@echo "Running $${testname}" >> gotools.sum; \
-@NATIVE_TRUE@echo "Running $${testname}" >> gotools.log; \
+@NATIVE_TRUE@echo "Running $${testname} ..." >> gotools.sum; \
+@NATIVE_TRUE@echo "Running $${testname} ..." >> gotools.log; \
@NATIVE_TRUE@sed -e 's/^--- \(.*\) ([^)]*)$$/\1/' < $${file} >>
gotools.log; \
@NATIVE_TRUE@grep '^--- ' $${file} | sed -e 's/^--- \(.*\) ([^)]*)$$/\1/'
-e 's/SKIP/UNTESTED/' | sort -k 2 >> gotools.sum; \
@NATIVE_TRUE@ done
--
2.34.1
Re: [PATCH] Format gotools.sum closer to what DejaGnu does
> On Nov 2, 2023, at 21:02, rep.dot@gmail.com wrote:
>
> Hi Maxim!
>
> Many thanks for the patch! Quick question below..
>
> On 2 November 2023 13:48:55 CET, Maxim Kuvyrkov
> wrote:
>> ... to restore compatability with validate_failures.py .
>> The testsuite script validate_failures.py expects
>> "Running ..." to extract values,
>> and gotools.sum provided "Running ".
>>
>> Note that libgo.sum, which also uses Makefile logic to generate
>> DejaGnu-like output, already has "..." suffix.
>>
>> gotools/ChangeLog:
>>
>> * Makefile.am: Update "Running ..." output
>> * Makefile.in: Regenerate.
>> ---
>> gotools/Makefile.am | 4 ++--
>> gotools/Makefile.in | 5 +++--
>> 2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/gotools/Makefile.am b/gotools/Makefile.am
>> index 7b5302990f8..d2376b9c25b 100644
>> --- a/gotools/Makefile.am
>> +++ b/gotools/Makefile.am
>> @@ -332,8 +332,8 @@ check: check-head check-go-tool check-runtime
>> check-cgo-test check-carchive-test
>> @cp gotools.sum gotools.log
>> @for file in cmd_go-testlog runtime-testlog cgo-testlog carchive-testlog
>> cmd_vet-testlog embed-testlog; do \
>> testname=`echo $${file} | sed -e 's/-testlog//' -e 's|_|/|'`; \
>> - echo "Running $${testname}" >> gotools.sum; \
>> - echo "Running $${testname}" >> gotools.log; \
>> + echo "Running $${testname} ..." >> gotools.sum; \
>> + echo "Running $${testname} ..." >> gotools.log; \
>> sed -e 's/^--- \(.*\) ([^)]*)$$/\1/' < $${file} >> gotools.log; \
>> grep '^--- ' $${file} | sed -e 's/^--- \(.*\) ([^)]*)$$/\1/' -e
>> 's/SKIP/UNTESTED/' | sort -k 2 >> gotools.sum; \
>> done
>> diff --git a/gotools/Makefile.in b/gotools/Makefile.in
>> index 2783b91ef4b..9cc238e748d 100644
>> --- a/gotools/Makefile.in
>> +++ b/gotools/Makefile.in
>> @@ -317,6 +317,7 @@ pdfdir = @pdfdir@
>> prefix = @prefix@
>> program_transform_name = @program_transform_name@
>> psdir = @psdir@
>> +runstatedir = @runstatedir@
>
> Are you sure you used the correct version of automake?
I used automake 1.15.1 (from Ubuntu 20.04 automake-1.15 package), and I
double-checked after getting the runstatedir update.
I would appreciate someone checking on their side to make sure I don't have
something weird going on in my setup.
--
Maxim Kuvyrkov
https://www.linaro.org
Re: [PATCH 01/12] [contrib] validate_failures.py: Avoid testsuite aliasing
Patch proposed at https://gcc.gnu.org/pipermail/gcc-patches/2023-November/635000.html -- Maxim Kuvyrkov https://www.linaro.org > On Sep 27, 2023, at 18:47, Maxim Kuvyrkov wrote: > > Hi Bernhard, > > Thanks, I meant to fix this, but forgot. > > The underlying problem here is that we want to detect which sub-testsuites > had failures. Current regex doesn't match go's case because there is no > "..." at the end: "Running foo" vs "Running foo ..." . > > My preferred way of fixing this is to make go's testsuite print out "..." . > We have a similar patch for glibc [1]. > > [1] https://sourceware.org/pipermail/libc-alpha/2023-June/148702.html > > -- > Maxim Kuvyrkov > https://www.linaro.org > >> On Sep 26, 2023, at 19:46, Bernhard Reutner-Fischer >> wrote: >> >> Hi Maxim! >> >> On Mon, 5 Jun 2023 18:06:25 +0400 >> Maxim Kuvyrkov via Gcc-patches wrote: >> >>>> On Jun 3, 2023, at 19:17, Jeff Law wrote: >>>> >>>> On 6/2/23 09:20, Maxim Kuvyrkov via Gcc-patches wrote: >>>>> This patch adds tracking of current testsuite "tool" and "exp" >>>>> to the processing of .sum files. This avoids aliasing between >>>>> tests from different testsuites with same name+description. >>>>> E.g., this is necessary for testsuite/c-c++-common, which is ran >>>>> for both gcc and g++ "tools". >>>>> This patch changes manifest format from ... >>>>> >>>>> FAIL: gcc_test >>>>> FAIL: g++_test >>>>> >>>>> ... to ... >>>>> >>>>> === gcc tests === >>>>> Running gcc/foo.exp ... >>>>> FAIL: gcc_test >>>>> === gcc Summary == >>>>> === g++ tests === >>>>> Running g++/bar.exp ... >>>>> FAIL: g++_test >>>>> === g++ Summary == >>>>> . >>>>> The new format uses same formatting as DejaGnu's .sum files >>>>> to specify which "tool" and "exp" the test belongs to. >>>> I think the series is fine. You're not likely to hear from Diego or Doug >>>> I suspect, I don't think either are involved in GNU stuff anymore. >>>> >>> >>> Thanks, Jeff. I'll wait for a couple of days and will merge if there are >>> no new comments. >> >> Maxim, may i ask you to have a look at the following problem, please? >> >> ISTM that your exp code does not work as expected for go, maybe you >> forgot to test the changes with go enabled? >> >> Ever since your changes in summer i see the following: >> >> gcc-14.mine$ >> /scratch/src/gcc-14.mine/contrib/testsuite-management/validate_failures.py >> --clean_build ../gcc-14.orig/ >> Getting actual results from build directory . >> ./gcc/testsuite/go/go.sum >> ./gcc/testsuite/gcc/gcc.sum >> ./gcc/testsuite/objc/objc.sum >> ./gcc/testsuite/jit/jit.sum >> ./gcc/testsuite/gdc/gdc.sum >> ./gcc/testsuite/gnat/gnat.sum >> ./gcc/testsuite/ada/acats/acats.sum >> ./gcc/testsuite/g++/g++.sum >> ./gcc/testsuite/obj-c++/obj-c++.sum >> ./gcc/testsuite/rust/rust.sum >> ./gcc/testsuite/gfortran/gfortran.sum >> ./x86_64-pc-linux-gnu/libgomp/testsuite/libgomp.sum >> ./x86_64-pc-linux-gnu/libphobos/testsuite/libphobos.sum >> ./x86_64-pc-linux-gnu/libstdc++-v3/testsuite/libstdc++.sum >> ./x86_64-pc-linux-gnu/libffi/testsuite/libffi.sum >> ./x86_64-pc-linux-gnu/libitm/testsuite/libitm.sum >> ./x86_64-pc-linux-gnu/libgo/libgo.sum >> ./x86_64-pc-linux-gnu/libatomic/testsuite/libatomic.sum >> ./gotools/gotools.sum >> .sum file seems to be broken: tool="gotools", exp="None", >> summary_line="FAIL: TestScript" >> Traceback (most recent call last): >> File >> "/scratch/src/gcc-14.mine/contrib/testsuite-management/validate_failures.py", >> line 732, in >> retval = Main(sys.argv) >> File >> "/scratch/src/gcc-14.mine/contrib/testsuite-management/validate_failures.py", >> line 721, in Main >> retval = CompareBuilds() >> File >> "/scratch/src/gcc-14.mine/contrib/testsuite-management/validate_failures.py", >> line 622, in CompareBuilds >> actual = GetResults(sum_files) >> File >> "/scratch/src/gcc-14.mine/contrib/testsuite-management/validate_failures.py", >> line 466, in GetResults >> build_results.update(ParseSummary(s
[PATCH] Format gotools.sum closer to what DejaGnu does
... to restore compatability with validate_failures.py .
The testsuite script validate_failures.py expects
"Running ..." to extract values,
and gotools.sum provided "Running ".
Note that libgo.sum, which also uses Makefile logic to generate
DejaGnu-like output, already has "..." suffix.
gotools/ChangeLog:
* Makefile.am: Update "Running ..." output
* Makefile.in: Regenerate.
---
gotools/Makefile.am | 4 ++--
gotools/Makefile.in | 5 +++--
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/gotools/Makefile.am b/gotools/Makefile.am
index 7b5302990f8..d2376b9c25b 100644
--- a/gotools/Makefile.am
+++ b/gotools/Makefile.am
@@ -332,8 +332,8 @@ check: check-head check-go-tool check-runtime
check-cgo-test check-carchive-test
@cp gotools.sum gotools.log
@for file in cmd_go-testlog runtime-testlog cgo-testlog
carchive-testlog cmd_vet-testlog embed-testlog; do \
testname=`echo $${file} | sed -e 's/-testlog//' -e 's|_|/|'`; \
- echo "Running $${testname}" >> gotools.sum; \
- echo "Running $${testname}" >> gotools.log; \
+ echo "Running $${testname} ..." >> gotools.sum; \
+ echo "Running $${testname} ..." >> gotools.log; \
sed -e 's/^--- \(.*\) ([^)]*)$$/\1/' < $${file} >> gotools.log; \
grep '^--- ' $${file} | sed -e 's/^--- \(.*\) ([^)]*)$$/\1/' -e
's/SKIP/UNTESTED/' | sort -k 2 >> gotools.sum; \
done
diff --git a/gotools/Makefile.in b/gotools/Makefile.in
index 2783b91ef4b..9cc238e748d 100644
--- a/gotools/Makefile.in
+++ b/gotools/Makefile.in
@@ -317,6 +317,7 @@ pdfdir = @pdfdir@
prefix = @prefix@
program_transform_name = @program_transform_name@
psdir = @psdir@
+runstatedir = @runstatedir@
sbindir = @sbindir@
sharedstatedir = @sharedstatedir@
srcdir = @srcdir@
@@ -1003,8 +1004,8 @@ mostlyclean-local:
@NATIVE_TRUE@ @cp gotools.sum gotools.log
@NATIVE_TRUE@ @for file in cmd_go-testlog runtime-testlog cgo-testlog
carchive-testlog cmd_vet-testlog embed-testlog; do \
@NATIVE_TRUE@testname=`echo $${file} | sed -e 's/-testlog//' -e 's|_|/|'`;
\
-@NATIVE_TRUE@echo "Running $${testname}" >> gotools.sum; \
-@NATIVE_TRUE@echo "Running $${testname}" >> gotools.log; \
+@NATIVE_TRUE@echo "Running $${testname} ..." >> gotools.sum; \
+@NATIVE_TRUE@echo "Running $${testname} ..." >> gotools.log; \
@NATIVE_TRUE@sed -e 's/^--- \(.*\) ([^)]*)$$/\1/' < $${file} >>
gotools.log; \
@NATIVE_TRUE@grep '^--- ' $${file} | sed -e 's/^--- \(.*\) ([^)]*)$$/\1/'
-e 's/SKIP/UNTESTED/' | sort -k 2 >> gotools.sum; \
@NATIVE_TRUE@ done
--
2.34.1
Re: [PATCH v2] ARM: Block predication on atomics [PR111235]
> On Oct 1, 2023, at 00:36, Ramana Radhakrishnan > wrote: > > + linaro-toolchain as I don't understand the CI issues on patchwork. > > ... > Ok if no regressions but as you might get nagged by the post commit CI ... I don't see any pre-commit failures for this patch, but regardless of what results are for pre-commit CI, there's always a chance to identify problems in post-commit CI -- simply because we test wa-a-ay more configurations in post-commit CI than in pre-commit CI. > > While it is not policy yet to look at these bots but given the > enthusiasm at Cauldron for patchwork and pre-commit CI and because all > my armhf boxes are boxed up, I decided to do something a bit novel ! > > I tried reviewing this via patchwork > > https://patchwork.sourceware.org/project/gcc/patch/pawpr08mb8982a6aa40749b74cad14c5783...@pawpr08mb8982.eurprd08.prod.outlook.com/ > > and notice that > > https://ci.linaro.org/job/tcwg_gcc_build--master-arm-precommit/2393/artifact/artifacts/artifacts.precommit/notify/mail-body.txt > says nothing could be built. Um, no. This says ... === Results changed to # reset_artifacts: -10 # true: 0 # build_abe gcc: 1 From # reset_artifacts: -10 # true: 0 # build_abe gcc: 1 === ... i.e., build succeeded both before and after patch. We'll change the boilerplate intro for successful builds from ... "Dear contributor, our automatic CI has detected problems related to your patch(es)." ... to ... "Dear contributor, you are awesome, no CI failures related to your patch(es)". One things that is strange -- testsuite builds were not triggered, we have only 2 reports from build tests, but are missing another 2 reports from testsuite tests. > > Possibly worth double checking the status for it being a false > negative as to why the build failed. Pre-commit CI is happy with the patch, albeit testsuite checks didn't run for some reason. Regardless, we'll quickly catch and report any fallout in the post-commit CI once the patch is merged. > > It was green on patchwork but remembering that Green is not Green for > CI in patchwork I clicked on the afore mentioned ci.linaro.org link > and see that it's actually broken. Unfortunately, I seem to have confused developers about green and red at my Cauldron presentation. "Green/Red" in patchwork mean the usual PASS/FAIL. It's only in post-commit CI in jenkins interface green and red mean something different. -- Maxim Kuvyrkov https://www.linaro.org
Re: [PATCH 01/12] [contrib] validate_failures.py: Avoid testsuite aliasing
Hi Bernhard, Thanks, I meant to fix this, but forgot. The underlying problem here is that we want to detect which sub-testsuites had failures. Current regex doesn't match go's case because there is no "..." at the end: "Running foo" vs "Running foo ..." . My preferred way of fixing this is to make go's testsuite print out "..." . We have a similar patch for glibc [1]. [1] https://sourceware.org/pipermail/libc-alpha/2023-June/148702.html -- Maxim Kuvyrkov https://www.linaro.org > On Sep 26, 2023, at 19:46, Bernhard Reutner-Fischer > wrote: > > Hi Maxim! > > On Mon, 5 Jun 2023 18:06:25 +0400 > Maxim Kuvyrkov via Gcc-patches wrote: > >>> On Jun 3, 2023, at 19:17, Jeff Law wrote: >>> >>> On 6/2/23 09:20, Maxim Kuvyrkov via Gcc-patches wrote: >>>> This patch adds tracking of current testsuite "tool" and "exp" >>>> to the processing of .sum files. This avoids aliasing between >>>> tests from different testsuites with same name+description. >>>> E.g., this is necessary for testsuite/c-c++-common, which is ran >>>> for both gcc and g++ "tools". >>>> This patch changes manifest format from ... >>>> >>>> FAIL: gcc_test >>>> FAIL: g++_test >>>> >>>> ... to ... >>>> >>>> === gcc tests === >>>> Running gcc/foo.exp ... >>>> FAIL: gcc_test >>>> === gcc Summary == >>>> === g++ tests === >>>> Running g++/bar.exp ... >>>> FAIL: g++_test >>>> === g++ Summary == >>>> . >>>> The new format uses same formatting as DejaGnu's .sum files >>>> to specify which "tool" and "exp" the test belongs to. >>> I think the series is fine. You're not likely to hear from Diego or Doug I >>> suspect, I don't think either are involved in GNU stuff anymore. >>> >> >> Thanks, Jeff. I'll wait for a couple of days and will merge if there are no >> new comments. > > Maxim, may i ask you to have a look at the following problem, please? > > ISTM that your exp code does not work as expected for go, maybe you > forgot to test the changes with go enabled? > > Ever since your changes in summer i see the following: > > gcc-14.mine$ > /scratch/src/gcc-14.mine/contrib/testsuite-management/validate_failures.py > --clean_build ../gcc-14.orig/ > Getting actual results from build directory . > ./gcc/testsuite/go/go.sum > ./gcc/testsuite/gcc/gcc.sum > ./gcc/testsuite/objc/objc.sum > ./gcc/testsuite/jit/jit.sum > ./gcc/testsuite/gdc/gdc.sum > ./gcc/testsuite/gnat/gnat.sum > ./gcc/testsuite/ada/acats/acats.sum > ./gcc/testsuite/g++/g++.sum > ./gcc/testsuite/obj-c++/obj-c++.sum > ./gcc/testsuite/rust/rust.sum > ./gcc/testsuite/gfortran/gfortran.sum > ./x86_64-pc-linux-gnu/libgomp/testsuite/libgomp.sum > ./x86_64-pc-linux-gnu/libphobos/testsuite/libphobos.sum > ./x86_64-pc-linux-gnu/libstdc++-v3/testsuite/libstdc++.sum > ./x86_64-pc-linux-gnu/libffi/testsuite/libffi.sum > ./x86_64-pc-linux-gnu/libitm/testsuite/libitm.sum > ./x86_64-pc-linux-gnu/libgo/libgo.sum > ./x86_64-pc-linux-gnu/libatomic/testsuite/libatomic.sum > ./gotools/gotools.sum > .sum file seems to be broken: tool="gotools", exp="None", summary_line="FAIL: > TestScript" > Traceback (most recent call last): > File > "/scratch/src/gcc-14.mine/contrib/testsuite-management/validate_failures.py", > line 732, in >retval = Main(sys.argv) > File > "/scratch/src/gcc-14.mine/contrib/testsuite-management/validate_failures.py", > line 721, in Main >retval = CompareBuilds() > File > "/scratch/src/gcc-14.mine/contrib/testsuite-management/validate_failures.py", > line 622, in CompareBuilds >actual = GetResults(sum_files) > File > "/scratch/src/gcc-14.mine/contrib/testsuite-management/validate_failures.py", > line 466, in GetResults >build_results.update(ParseSummary(sum_fname)) > File > "/scratch/src/gcc-14.mine/contrib/testsuite-management/validate_failures.py", > line 405, in ParseSummary >result = result_set.MakeTestResult(line, ordinal) > File > "/scratch/src/gcc-14.mine/contrib/testsuite-management/validate_failures.py", > line 239, in MakeTestResult >return TestResult(summary_line, ordinal, > File > "/scratch/src/gcc-14.mine/contrib/testsuite-management/validate_failures.py", > line 151, in __init__ >raise > RuntimeError: No active exception to reraise > > > The problem seems to be that gotools.sum does not mention any ".exp" > files. > > $ grep "Running " gotools/gotools.sum > Running cmd/go > Running runtime > Running cgo > Running carchive > Running cmd/vet > Running embed > $ grep -c "\.exp" gotools/gotools.sum > 0 > > The .sum files looks like this: > ---8<--- > Test Run By foo on Tue Sep 26 14:46:48 CEST 2023 > Native configuration is x86_64-foo-linux-gnu > >=== gotools tests === > > Running cmd/go > UNTESTED: TestAccidentalGitCheckout > PASS: TestAlwaysLinkSysoFiles > ... > UNTESTED: TestParallelTest > FAIL: TestScript > ... > ---8<--- > > May i ask you to have a look, please? > > TIA,
Re: PATCH v6 4/4] ree: Improve ree pass for rs6000 target using defined ABI interfaces.
Hi Ajit,
Is this patch supposed to be applied on top of another patch? As is, this
patch fails build on AArch64 and AArch32, and Linaro TCWG CI have sent
notifications about the failures for v5 [1] and v6 [2] of this patch to you.
Did you receive the notifications?
Kind regards,
[1]
https://patchwork.sourceware.org/project/gcc/patch/5ad7cdca-63e1-73af-b38d-d58898e21...@linux.ibm.com/
[2]
https://patchwork.sourceware.org/project/gcc/patch/65ed79a3-9964-dd50-39cb-98d5dbc72...@linux.ibm.com/
--
Maxim Kuvyrkov
https://www.linaro.org
> On Sep 18, 2023, at 09:59, Ajit Agarwal via Gcc-patches
> wrote:
>
> This new version of patch 6 use improve ree pass for rs6000 target using
> defined ABI interfaces.
> Bootstrapped and regtested on power64-linux-gnu.
>
> Review comments incorporated.
>
> Thanks & Regards
> Ajit
>
>
> ree: Improve ree pass for rs6000 target using defined abi interfaces
>
> For rs6000 target we see redundant zero and sign extension and done to
> improve ree pass to eliminate such redundant zero and sign extension
> using defined ABI interfaces.
>
> 2023-09-18 Ajit Kumar Agarwal
>
> gcc/ChangeLog:
>
> * ree.cc (combine_reaching_defs): Use of zero_extend and sign_extend
> defined abi interfaces.
> (add_removable_extension): Use of defined abi interfaces for no
> reaching defs.
> (abi_extension_candidate_return_reg_p): New function.
> (abi_extension_candidate_p): New function.
> (abi_extension_candidate_argno_p): New function.
> (abi_handle_regs_without_defs_p): New function.
> (abi_target_promote_function_mode): New function.
>
> gcc/testsuite/ChangeLog:
>
>* g++.target/powerpc/zext-elim-3.C
> ---
> gcc/ree.cc| 145 +-
> .../g++.target/powerpc/zext-elim-3.C | 13 ++
> 2 files changed, 155 insertions(+), 3 deletions(-)
> create mode 100644 gcc/testsuite/g++.target/powerpc/zext-elim-3.C
>
> diff --git a/gcc/ree.cc b/gcc/ree.cc
> index fc04249fa84..e395af6b1bd 100644
> --- a/gcc/ree.cc
> +++ b/gcc/ree.cc
> @@ -514,7 +514,8 @@ get_uses (rtx_insn *insn, rtx reg)
> if (REGNO (DF_REF_REG (def)) == REGNO (reg))
> break;
>
> - gcc_assert (def != NULL);
> + if (def == NULL)
> +return NULL;
>
> ref_chain = DF_REF_CHAIN (def);
>
> @@ -750,6 +751,118 @@ get_extended_src_reg (rtx src)
> return src;
> }
>
> +/* Return TRUE if target mode is equal to source mode of zero_extend
> + or sign_extend otherwise false. */
> +
> +static bool
> +abi_target_promote_function_mode (machine_mode mode)
> +{
> + int unsignedp;
> + machine_mode tgt_mode =
> +targetm.calls.promote_function_mode (NULL_TREE, mode, ,
> + NULL_TREE, 1);
> +
> + if (tgt_mode == mode)
> +return true;
> + else
> +return false;
> +}
> +
> +/* Return TRUE if the candidate insn is zero extend and regno is
> + an return registers. */
> +
> +static bool
> +abi_extension_candidate_return_reg_p (rtx_insn *insn, int regno)
> +{
> + rtx set = single_set (insn);
> +
> + if (GET_CODE (SET_SRC (set)) != ZERO_EXTEND)
> +return false;
> +
> + if (FUNCTION_VALUE_REGNO_P (regno))
> +return true;
> +
> + return false;
> +}
> +
> +/* Return TRUE if reg source operand of zero_extend is argument registers
> + and not return registers and source and destination operand are same
> + and mode of source and destination operand are not same. */
> +
> +static bool
> +abi_extension_candidate_p (rtx_insn *insn)
> +{
> + rtx set = single_set (insn);
> +
> + if (GET_CODE (SET_SRC (set)) != ZERO_EXTEND)
> +return false;
> +
> + machine_mode ext_dst_mode = GET_MODE (SET_DEST (set));
> + rtx orig_src = XEXP (SET_SRC (set),0);
> +
> + bool copy_needed
> += (REGNO (SET_DEST (set)) != REGNO (XEXP (SET_SRC (set), 0)));
> +
> + if (!copy_needed && ext_dst_mode != GET_MODE (orig_src)
> + && FUNCTION_ARG_REGNO_P (REGNO (orig_src))
> + && !abi_extension_candidate_return_reg_p (insn, REGNO (orig_src)))
> +return true;
> +
> + return false;
> +}
> +
> +/* Return TRUE if the candidate insn is zero extend and regno is
> + an argument registers. */
> +
> +static bool
> +abi_extension_candidate_argno_p (rtx_code code, int regno)
> +{
> + if (code != ZERO_EXTEND)
> +return false;
> +
> + if (FUNCTION_ARG_REGNO_P (regno))
> +return true;
> +
> + return false;
> +}
> +
> +/* Return TRUE if the candidate insn doesn't have defs and have
> + * uses without RTX_BIN_ARITH/RTX_COMM_ARITH/RTX_UNARY rtx class. */
> +
> +static bool
> +abi_handle_regs_without_d
Re: [Linaro-TCWG-CI] gcc patch #75674: FAIL: 68 regressions
Hi Everyone, Normally, notifications from Linaro TCWG precommit CI are sent only to patch author and patch submitter. In this case the sender was rewritten to "Benjamin Priour via Gcc-patches ", which was detected by Patchwork [1] as patch submitter. Hi Mark, Is "From:" re-write on gcc-patches@ mailing list a side-effect of [2]? I see that some, but not all messages to gcc-patches@ have their "From:" re-written. Also, do you know if re-write of "From:" on gcc-patches@ is expected? [1] https://patchwork.sourceware.org/project/gcc/list/ [2] https://sourceware.org/bugzilla/show_bug.cgi?id=29713 Thanks! -- Maxim Kuvyrkov https://www.linaro.org > On Sep 12, 2023, at 02:58, ci_not...@linaro.org wrote: > > Dear contributor, our automatic CI has detected problems related to your > patch(es). Please find some details below. If you have any questions, > please follow up on linaro-toolch...@lists.linaro.org mailing list, Libera's > #linaro-tcwg channel, or ping your favourite Linaro toolchain developer on > the usual project channel. > > In CI config tcwg_gcc_check/master-aarch64 after: > > | gcc patch https://patchwork.sourceware.org/patch/75674 > | Author: benjamin priour > | Date: Mon Sep 11 19:44:34 2023 +0200 > | > | analyzer: Move gcc.dg/analyzer tests to c-c++-common (3) [PR96395] > | > | Hi, > | > | Patch below is mostly done, just have to check the formatting. > | Althought, I'd like your feedback on how to manage named_constants > | from enum in C++. > | ... 21 lines of the commit log omitted. > | ... applied on top of baseline commit: > | 048927ed856 i386: Handle CONST_WIDE_INT in output_pic_addr_const [PR111340] > > FAIL: 68 regressions > > regressions.sum: > === g++ tests === > > Running g++:g++.dg/analyzer/analyzer.exp ... > FAIL: c-c++-common/analyzer/call-summaries-2.c -std=c++14 (test for excess > errors) > FAIL: c-c++-common/analyzer/call-summaries-2.c -std=c++17 (test for excess > errors) > FAIL: c-c++-common/analyzer/call-summaries-2.c -std=c++20 (test for excess > errors) > FAIL: c-c++-common/analyzer/call-summaries-2.c -std=c++98 (test for excess > errors) > FAIL: c-c++-common/analyzer/memcpy-1.c -std=c++14 (test for warnings, line > 25) > FAIL: c-c++-common/analyzer/memcpy-1.c -std=c++14 (test for warnings, line > 48) > FAIL: c-c++-common/analyzer/memcpy-1.c -std=c++14 (test for excess errors) > ... and 63 more entries > > You can find the failure logs in *.log.1.xz files in > - > https://ci.linaro.org/job/tcwg_gcc_check--master-aarch64-precommit/2244/artifact/artifacts/artifacts.precommit/00-sumfiles/ > . > The full lists of regressions and progressions are in > - > https://ci.linaro.org/job/tcwg_gcc_check--master-aarch64-precommit/2244/artifact/artifacts/artifacts.precommit/notify/ > . > The list of [ignored] baseline and flaky failures are in > - > https://ci.linaro.org/job/tcwg_gcc_check--master-aarch64-precommit/2244/artifact/artifacts/artifacts.precommit/sumfiles/xfails.xfail > . > > > > -8<--8<--8<-- > The information below can be used to reproduce a debug environment: > > Current build : > https://ci.linaro.org/job/tcwg_gcc_check--master-aarch64-precommit/2244/artifact/artifacts > Reference build : > https://ci.linaro.org/job/tcwg_gcc_check--master-aarch64-build/927/artifact/artifacts
Re: [PATCH v6 0/4] P1689R5 support
> On Jun 7, 2023, at 00:50, Ben Boeckel via Gcc-patches > wrote: > > Hi, > > This patch series adds initial support for ISO C++'s [P1689R5][], a > format for describing C++ module requirements and provisions based on > the source code. This is required because compiling C++ with modules is > not embarrassingly parallel and need to be ordered to ensure that > `import some_module;` can be satisfied in time by making sure that any > TU with `export import some_module;` is compiled first. Hi Ben, This patch series causes ICEs on arm-linux-gnueabihf. Would you please investigate? Please let me know if you need any in reproducing these. === g++ tests === Running g++:g++.dg/modules/modules.exp ... FAIL: g++.dg/modules/ben-1_a.C -std=c++17 (internal compiler error: Segmentation fault) FAIL: g++.dg/modules/ben-1_a.C -std=c++17 (test for excess errors) FAIL: g++.dg/modules/ben-1_a.C -std=c++2a (internal compiler error: Segmentation fault) FAIL: g++.dg/modules/ben-1_a.C -std=c++2a (test for excess errors) FAIL: g++.dg/modules/ben-1_a.C -std=c++2b (internal compiler error: Segmentation fault) FAIL: g++.dg/modules/ben-1_a.C -std=c++2b (test for excess errors) FAIL: g++.dg/modules/ben-1_a.C module-cmi =partitions/module-import.mod (partitions/module-import.mod) FAIL: g++.dg/modules/ben-1_b.C -std=c++17 (internal compiler error: Segmentation fault) FAIL: g++.dg/modules/ben-1_b.C -std=c++17 (test for excess errors) FAIL: g++.dg/modules/ben-1_b.C -std=c++2a (internal compiler error: Segmentation fault) FAIL: g++.dg/modules/ben-1_b.C -std=c++2a (test for excess errors) FAIL: g++.dg/modules/ben-1_b.C -std=c++2b (internal compiler error: Segmentation fault) FAIL: g++.dg/modules/ben-1_b.C -std=c++2b (test for excess errors) FAIL: g++.dg/modules/ben-1_b.C module-cmi =module.mod (module.mod) FAIL: g++.dg/modules/gc-2_a.C -std=c++17 (internal compiler error: Segmentation fault) FAIL: g++.dg/modules/gc-2_a.C -std=c++17 (test for excess errors) FAIL: g++.dg/modules/gc-2_a.C -std=c++2a (internal compiler error: Segmentation fault) FAIL: g++.dg/modules/gc-2_a.C -std=c++2a (test for excess errors) FAIL: g++.dg/modules/gc-2_a.C -std=c++2b (internal compiler error: Segmentation fault) FAIL: g++.dg/modules/gc-2_a.C -std=c++2b (test for excess errors) FAIL: g++.dg/modules/gc-2_a.C module-cmi =map-1_a.nms (map-1_a.nms) UNRESOLVED: g++.dg/modules/map-1 -std=c++17 execute UNRESOLVED: g++.dg/modules/map-1 -std=c++17 link UNRESOLVED: g++.dg/modules/map-1 -std=c++2a execute UNRESOLVED: g++.dg/modules/map-1 -std=c++2a link UNRESOLVED: g++.dg/modules/map-1 -std=c++2b execute UNRESOLVED: g++.dg/modules/map-1 -std=c++2b link FAIL: g++.dg/modules/map-1_a.C -std=c++17 (internal compiler error: Segmentation fault) FAIL: g++.dg/modules/map-1_a.C -std=c++17 (test for excess errors) FAIL: g++.dg/modules/map-1_a.C -std=c++2a (internal compiler error: Segmentation fault) FAIL: g++.dg/modules/map-1_a.C -std=c++2a (test for excess errors) FAIL: g++.dg/modules/map-1_a.C -std=c++2b (internal compiler error: Segmentation fault) FAIL: g++.dg/modules/map-1_a.C -std=c++2b (test for excess errors) FAIL: g++.dg/modules/map-1_a.C module-cmi =map-1_a.nms (map-1_a.nms) FAIL: g++.dg/modules/map-1_b.C -std=c++17 (internal compiler error: Segmentation fault) FAIL: g++.dg/modules/map-1_b.C -std=c++17 (test for excess errors) FAIL: g++.dg/modules/map-1_b.C -std=c++2a (internal compiler error: Segmentation fault) FAIL: g++.dg/modules/map-1_b.C -std=c++2a (test for excess errors) FAIL: g++.dg/modules/map-1_b.C -std=c++2b (internal compiler error: Segmentation fault) FAIL: g++.dg/modules/map-1_b.C -std=c++2b (test for excess errors) FAIL: g++.dg/modules/map-2.C -std=c++17 at line 8 (test for errors, line 7) FAIL: g++.dg/modules/map-2.C -std=c++17 at line 9 (test for errors, line ) FAIL: g++.dg/modules/map-2.C -std=c++17 (internal compiler error: Segmentation fault) FAIL: g++.dg/modules/map-2.C -std=c++17 (test for excess errors) FAIL: g++.dg/modules/map-2.C -std=c++2a at line 8 (test for errors, line 7) FAIL: g++.dg/modules/map-2.C -std=c++2a at line 9 (test for errors, line ) FAIL: g++.dg/modules/map-2.C -std=c++2a (internal compiler error: Segmentation fault) FAIL: g++.dg/modules/map-2.C -std=c++2a (test for excess errors) FAIL: g++.dg/modules/map-2.C -std=c++2b at line 8 (test for errors, line 7) FAIL: g++.dg/modules/map-2.C -std=c++2b at line 9 (test for errors, line ) FAIL: g++.dg/modules/map-2.C -std=c++2b (internal compiler error: Segmentation fault) FAIL: g++.dg/modules/map-2.C -std=c++2b (test for excess errors) === Thanks, -- Maxim Kuvyrkov https://www.linaro.org > > [P1689R5]: https://isocpp.org/files/papers/P1689R5.html > > I've also added patches to include imported module CMI files and the > module mapper file as dependencies of the compilation. I briefly looked > into adding dependencies on response files as well, but that appeared to > need some code contortio
Re: [PATCH] analyzer: Standalone OOB-warning [PR109437, PR109439]
> On Jun 6, 2023, at 15:48, Benjamin Priour via Gcc-patches > wrote: > > From: Benjamin Priour > > This patch enchances -Wanalyzer-out-of-bounds that is no longer paired > with a -Wanalyzer-use-of-uninitialized-value on out-of-bounds-read. > > This also fixes PR analyzer/109437. > Before there could always be at most one OOB-read warning per frame because > -Wanalyzer-use-of-uninitialized-value always terminates the analysis > path. > > PR analyzer/109439 > > gcc/analyzer/ChangeLog: > > * bounds-checking.cc (region_model::check_symbolic_bounds): > Returns whether the BASE_REG region access was OOB. > (region_model::check_region_bounds): Likewise. > * region-model.cc (region_model::get_store_value): Creates an > unknown svalue on OOB-read access to REG. > (region_model::check_region_access): Returns whether an > unknown svalue needs be created. > (region_model::check_region_for_read): Passes > check_region_access return value. > * region-model.h: Update prior function definitions. > > gcc/testsuite/ChangeLog: > > * gcc.dg/analyzer/out-of-bounds-2.c: Cleaned test for > uninitialized-value warning. > * gcc.dg/analyzer/out-of-bounds-5.c: Likewise. > * gcc.dg/analyzer/pr101962.c: Likewise. > * gcc.dg/analyzer/realloc-5.c: Likewise. > * gcc.dg/analyzer/pr109439.c: New test. > --- Hi Benjamin, This patch makes two tests fail on arm-linux-gnueabihf. Probably, they need to be updated as well. Would you please investigate? Let me know if it doesn't easily reproduce for you, and I'll help. === g++ tests === Running g++:g++.dg/analyzer/analyzer.exp ... FAIL: g++.dg/analyzer/pr100244.C -std=c++14 (test for warnings, line 17) FAIL: g++.dg/analyzer/pr100244.C -std=c++17 (test for warnings, line 17) FAIL: g++.dg/analyzer/pr100244.C -std=c++20 (test for warnings, line 17) === gcc tests === Running gcc:gcc.dg/analyzer/analyzer.exp ... FAIL: gcc.dg/analyzer/pr101962.c (test for warnings, line 19) Thanks, -- Maxim Kuvyrkov https://www.linaro.org > gcc/analyzer/bounds-checking.cc | 30 +-- > gcc/analyzer/region-model.cc | 22 +- > gcc/analyzer/region-model.h | 8 ++--- > .../gcc.dg/analyzer/out-of-bounds-2.c | 1 - > .../gcc.dg/analyzer/out-of-bounds-5.c | 2 -- > gcc/testsuite/gcc.dg/analyzer/pr101962.c | 1 - > gcc/testsuite/gcc.dg/analyzer/pr109439.c | 12 > gcc/testsuite/gcc.dg/analyzer/realloc-5.c | 1 - > 8 files changed, 51 insertions(+), 26 deletions(-) > create mode 100644 gcc/testsuite/gcc.dg/analyzer/pr109439.c > > diff --git a/gcc/analyzer/bounds-checking.cc b/gcc/analyzer/bounds-checking.cc > index 3bf542a8eba..479b8e4b88d 100644 > --- a/gcc/analyzer/bounds-checking.cc > +++ b/gcc/analyzer/bounds-checking.cc > @@ -767,9 +767,11 @@ public: > } > }; > > -/* Check whether an access is past the end of the BASE_REG. */ > +/* Check whether an access is past the end of the BASE_REG. > + Return TRUE if the access was valid, FALSE otherwise. > +*/ > > -void > +bool > region_model::check_symbolic_bounds (const region *base_reg, > const svalue *sym_byte_offset, > const svalue *num_bytes_sval, > @@ -800,6 +802,7 @@ region_model::check_symbolic_bounds (const region > *base_reg, > offset_tree, > num_bytes_tree, > capacity_tree)); > +return false; > break; > case DIR_WRITE: > ctxt->warn (make_unique (base_reg, > @@ -807,9 +810,11 @@ region_model::check_symbolic_bounds (const region > *base_reg, > offset_tree, > num_bytes_tree, > capacity_tree)); > +return false; > break; > } > } > + return true; > } > > static tree > @@ -822,9 +827,11 @@ maybe_get_integer_cst_tree (const svalue *sval) > return NULL_TREE; > } > > -/* May complain when the access on REG is out-of-bounds. */ > +/* May complain when the access on REG is out-of-bounds. > + Return TRUE if the access was valid, FALSE otherwise. > +*/ > > -void > +bool > region_model::check_region_bounds (const region *reg, > enum access_direction dir, > region_model_context *ctxt) const > @@ -839,14 +846,14 @@ region_model::check_region_bounds (const region *reg, > (e.g. because the analyzer did not see previous offsets on the latter, > it might think that a negative access is before the buffer). */ > if (base_reg->symbolic_p ()) > -return; > +return true; > > /* Find out how many bytes were accessed. */ > const svalue *num_bytes_sval = reg->get_byte_size_sval (m_mgr); > tree num_bytes_tree = maybe_get_integer_cst_tree (num_bytes_sval); > /* Bail out if 0 bytes are ac
Re: [PATCH] Handle FMA friendly in reassoc pass
> On May 25, 2023, at 03:30, Cui, Lili via Gcc-patches > wrote: > > From: Lili Cui > > Make some changes in reassoc pass to make it more friendly to fma pass later. > Using FMA instead of mult + add reduces register pressure and insruction > retired. > > There are mainly two changes > 1. Put no-mult ops and mult ops alternately at the end of the queue, which is > conducive to generating more fma and reducing the loss of FMA when breaking > the chain. > 2. Rewrite the rewrite_expr_tree_parallel function to try to build parallel > chains according to the given correlation width, keeping the FMA chance as > much as possible. > > With the patch applied > > On ICX: > 507.cactuBSSN_r: Improved by 1.7% for multi-copy . > 503.bwaves_r : Improved by 0.60% for single copy . > 507.cactuBSSN_r: Improved by 1.10% for single copy . > 519.lbm_r : Improved by 2.21% for single copy . > no measurable changes for other benchmarks. > > On aarch64 > 507.cactuBSSN_r: Improved by 1.7% for multi-copy. > 503.bwaves_r : Improved by 6.00% for single-copy. > no measurable changes for other benchmarks. Hi Cui, I'm seeing a 4% slowdown on 436.cactusADM from SPEC CPU2006 on aarch64-linux-gnu (Cortex-A57) when compiling with "-O2 -flto". All other benchmarks seem neutral to this patch, and I didn't observe the slow down with plain -O2 no-LTO or with -O3. Is this something interesting to investigate? I'll be happy to assist. Kind regards, -- Maxim Kuvyrkov https://www.linaro.org
Re: [committed] libstdc++: Fix code size regressions in std::vector [PR110060]
Hi Jonathan,
Interestingly, this increases code-size of -O3 code on aarch64-linux-gnu on
SPEC CPU2017's 641.leela_s benchmark [1].
In particular, FastBoard::get_nearby_enemies() grew from 1444 to 2212 bytes.
This seems like a corner-case; the rest of SPEC CPU2017 is, mostly, neutral to
this patch. Is this something you may be interested in investigating? I'll be
happy to assist.
Looking at assembly, one of the differences I see is that the "after" version
has calls to realloc_insert(), while "before" version seems to have them
inlined [2].
[1]
https://git.linaro.org/toolchain/ci/interesting-commits.git/tree/gcc/sha1/b7b255e77a271974479c34d1db3daafc04b920bc/tcwg_bmk-code_size-cpu2017fast/status.txt
[2] 641.leela_s is non-GPL/non-BSD benchmark, and I'm not sure if I can post
its compiled and/or preprocessed code publicly. I assume RedHat has SPEC
CPU2017 license, and I can post details to you privately.
Kind regards,
--
Maxim Kuvyrkov
https://www.linaro.org
> On Jun 1, 2023, at 19:09, Jonathan Wakely via Gcc-patches
> wrote:
>
> Tested powerpc64le-linux. Pusshed to trunk.
>
> -- >8 --
>
> My r14-1452-gfb409a15d9babc change to add optimization hints to
> std::vector causes regressions because it makes std::vector::size() and
> std::vector::capacity() too big to inline. That's the opposite of what
> I wanted, so revert the changes to those functions.
>
> To achieve the original aim of optimizing vec.assign(vec.size(), x) we
> can add a local optimization hint to _M_fill_assign, so that it doesn't
> affect all other uses of size() and capacity().
>
> Additionally, add the same hint to the _M_assign_aux overload for
> forward iterators and add that to the testcase.
>
> It would be nice to similarly optimize:
> if (vec1.size() == vec2.size()) vec1 = vec2;
> but adding hints to operator=(const vector&) doesn't help. Presumably
> the relationships between the two sizes and two capacities are too
> complex to track effectively.
>
> libstdc++-v3/ChangeLog:
>
> PR libstdc++/110060
> * include/bits/stl_vector.h (_Vector_base::_M_invariant):
> Remove.
> (vector::size, vector::capacity): Remove calls to _M_invariant.
> * include/bits/vector.tcc (vector::_M_fill_assign): Add
> optimization hint to reallocating path.
> (vector::_M_assign_aux(FwdIter, FwdIter, forward_iterator_tag)):
> Likewise.
> * testsuite/23_containers/vector/capacity/invariant.cc: Moved
> to...
> * testsuite/23_containers/vector/modifiers/assign/no_realloc.cc:
> ...here. Check assign(FwdIter, FwdIter) too.
> * testsuite/23_containers/vector/types/1.cc: Revert addition
> of -Wno-stringop-overread option.
> ---
> libstdc++-v3/include/bits/stl_vector.h| 23 +--
> libstdc++-v3/include/bits/vector.tcc | 17 ++
> .../assign/no_realloc.cc} | 6 +
> .../testsuite/23_containers/vector/types/1.cc | 2 +-
> 4 files changed, 20 insertions(+), 28 deletions(-)
> rename libstdc++-v3/testsuite/23_containers/vector/{capacity/invariant.cc =>
> modifiers/assign/no_realloc.cc} (70%)
>
> diff --git a/libstdc++-v3/include/bits/stl_vector.h
> b/libstdc++-v3/include/bits/stl_vector.h
> index e593be443bc..70ced3d101f 100644
> --- a/libstdc++-v3/include/bits/stl_vector.h
> +++ b/libstdc++-v3/include/bits/stl_vector.h
> @@ -389,23 +389,6 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
>
> protected:
>
> - __attribute__((__always_inline__))
> - _GLIBCXX20_CONSTEXPR void
> - _M_invariant() const
> - {
> -#if __OPTIMIZE__
> - if (this->_M_impl._M_finish < this->_M_impl._M_start)
> - __builtin_unreachable();
> - if (this->_M_impl._M_finish > this->_M_impl._M_end_of_storage)
> - __builtin_unreachable();
> -
> - size_t __sz = this->_M_impl._M_finish - this->_M_impl._M_start;
> - size_t __cap = this->_M_impl._M_end_of_storage - this->_M_impl._M_start;
> - if (__sz > __cap)
> - __builtin_unreachable();
> -#endif
> - }
> -
> _GLIBCXX20_CONSTEXPR
> void
> _M_create_storage(size_t __n)
> @@ -1005,10 +988,7 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
> size_type
> size() const _GLIBCXX_NOEXCEPT
> - {
> - _Base::_M_invariant();
> - return size_type(this->_M_impl._M_finish - this->_M_impl._M_start);
> - }
> + { return size_type(this->_M_impl._M_finish - this->_M_impl._M_start); }
>
> /** Returns the size() of the largest possible %vector. */
> _GLIBCXX_NODISCARD _GLIBCXX20_CONSTEXPR
> @@ -1095,7 +1075,6 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> size_type
> capacity() const _GLIBCXX_NOEXCEPT
>
Re: [r14-1579 Regression] FAIL: gfortran.dg/gomp/target-update-1.f90 -O scan-tree-dump gimple "#pragma omp target update to\\(c \\[len: [0-9]+\\]\\) to\\(present:a \\[len: [0-9]+\\]\\) to\\(e \\[len:
Hi Tobias,
We are also seeing this failure on arm-linux-gnueabihf.
It seems the problem is different order of variables in output:
- c a e d b
versus expected
- c a e b d
This is tree-dump output on arm-linux-gnueabihf:
#pragma omp target update to(c [len: 4]) to(present:a [len: 4000]) to(e
[len: 4]) from(d [len: 4]) from(present:b [len: 4000])
Let me know if you need any help in troubleshooting this.
--
Maxim Kuvyrkov
https://www.linaro.org
> On Jun 7, 2023, at 02:50, haochen.jiang via Gcc-patches
> wrote:
>
> On Linux/x86_64,
>
> 4ede915d5dde935a16df2c6640aee5ab22348d30 is the first bad commit
> commit 4ede915d5dde935a16df2c6640aee5ab22348d30
> Author: Tobias Burnus
> Date: Tue Jun 6 16:47:16 2023 +0200
>
>openmp: Add support for the 'present' modifier
>
> caused
>
> FAIL: gfortran.dg/gomp/target-update-1.f90 -O scan-tree-dump gimple
> "#pragma omp target update to\\(c \\[len: [0-9]+\\]\\) to\\(present:a \\[len:
> [0-9]+\\]\\) to\\(e \\[len: [0-9]+\\]\\) from\\(present:b \\[len:
> [0-9]+\\]\\) from\\(d \\[len: [0-9]+\\]\\)"
>
> with GCC configured with
>
> ../../gcc/configure
> --prefix=/export/users/haochenj/src/gcc-bisect/master/master/r14-1579/usr
> --enable-clocale=gnu --with-system-zlib --with-demangler-in-ld
> --with-fpmath=sse --enable-languages=c,c++,fortran --enable-cet --without-isl
> --enable-libmpx x86_64-linux --disable-bootstrap
>
> To reproduce:
>
> $ cd {build_dir}/gcc && make check
> RUNTESTFLAGS="gomp.exp=gfortran.dg/gomp/target-update-1.f90
> --target_board='unix{-m32}'"
> $ cd {build_dir}/gcc && make check
> RUNTESTFLAGS="gomp.exp=gfortran.dg/gomp/target-update-1.f90
> --target_board='unix{-m32\ -march=cascadelake}'"
> $ cd {build_dir}/gcc && make check
> RUNTESTFLAGS="gomp.exp=gfortran.dg/gomp/target-update-1.f90
> --target_board='unix{-m64}'"
> $ cd {build_dir}/gcc && make check
> RUNTESTFLAGS="gomp.exp=gfortran.dg/gomp/target-update-1.f90
> --target_board='unix{-m64\ -march=cascadelake}'"
>
> (Please do not reply to this email, for question about this report, contact
> me at haochen dot jiang at intel.com)
Re: [PATCH 01/12] [contrib] validate_failures.py: Avoid testsuite aliasing
> On Jun 3, 2023, at 19:17, Jeff Law wrote: > > On 6/2/23 09:20, Maxim Kuvyrkov via Gcc-patches wrote: >> This patch adds tracking of current testsuite "tool" and "exp" >> to the processing of .sum files. This avoids aliasing between >> tests from different testsuites with same name+description. >> E.g., this is necessary for testsuite/c-c++-common, which is ran >> for both gcc and g++ "tools". >> This patch changes manifest format from ... >> >> FAIL: gcc_test >> FAIL: g++_test >> >> ... to ... >> >> === gcc tests === >> Running gcc/foo.exp ... >> FAIL: gcc_test >> === gcc Summary == >> === g++ tests === >> Running g++/bar.exp ... >> FAIL: g++_test >> === g++ Summary == >> . >> The new format uses same formatting as DejaGnu's .sum files >> to specify which "tool" and "exp" the test belongs to. > I think the series is fine. You're not likely to hear from Diego or Doug I > suspect, I don't think either are involved in GNU stuff anymore. > Thanks, Jeff. I'll wait for a couple of days and will merge if there are no new comments. Kind regards, -- Maxim Kuvyrkov https://www.linaro.org
Re: [pushed] analyzer: implement various atomic builtins [PR109015]
Hi David,
Hm, I'm seeing this failure only in pre-commit testing, but I don't see it in
our post-commit testing of gcc:master.
Does this patch rely on your other patch committed just before this one?
--
Maxim Kuvyrkov
https://www.linaro.org
> On Jun 3, 2023, at 09:23, Maxim Kuvyrkov wrote:
>
> Hi David,
>
> The new test ICEs the compiler on aarch64-linux-gnu [1]. Would you please
> investigate?
>
> Running gcc:gcc.dg/analyzer/analyzer.exp ...
> FAIL: gcc.dg/analyzer/atomic-builtins-qemu-sockets.c (internal compiler
> error: in validate, at analyzer/store.cc:1329)
> FAIL: gcc.dg/analyzer/atomic-builtins-qemu-sockets.c (test for excess errors)
>
> This is a simple native build on aarch64-linux-gnu. Please let me know if
> you need any help in reproducing this.
>
> [1]
> https://ci.linaro.org/job/tcwg_gcc_check--master-aarch64-build/82/artifact/artifacts/artifacts.precommit/06-check_regression/results.compare/*view*/
>
> Thanks!
>
> --
> Maxim Kuvyrkov
> https://www.linaro.org
>
>
>
>
>> On Jun 2, 2023, at 17:32, David Malcolm via Gcc-patches
>> wrote:
>>
>> This patch implements many of the __atomic_* builtins from
>> sync-builtins.def as known_function subclasses within the analyzer.
>>
>> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
>> Pushed to trunk as r14-1497-gef768035ae8090.
>>
>> gcc/analyzer/ChangeLog:
>> PR analyzer/109015
>> * kf.cc (class kf_atomic_exchange): New.
>> (class kf_atomic_exchange_n): New.
>> (class kf_atomic_fetch_op): New.
>> (class kf_atomic_op_fetch): New.
>> (class kf_atomic_load): New.
>> (class kf_atomic_load_n): New.
>> (class kf_atomic_store_n): New.
>> (register_atomic_builtins): New function.
>> (register_known_functions): Call register_atomic_builtins.
>>
>> gcc/testsuite/ChangeLog:
>> PR analyzer/109015
>> * gcc.dg/analyzer/atomic-builtins-1.c: New test.
>> * gcc.dg/analyzer/atomic-builtins-haproxy-proxy.c: New test.
>> * gcc.dg/analyzer/atomic-builtins-qemu-sockets.c: New test.
>> * gcc.dg/analyzer/atomic-types-1.c: New test.
>> ---
>> gcc/analyzer/kf.cc| 355
>> .../gcc.dg/analyzer/atomic-builtins-1.c | 544 ++
>> .../analyzer/atomic-builtins-haproxy-proxy.c | 55 ++
>> .../analyzer/atomic-builtins-qemu-sockets.c | 18 +
>> .../gcc.dg/analyzer/atomic-types-1.c | 11 +
>> 5 files changed, 983 insertions(+)
>> create mode 100644 gcc/testsuite/gcc.dg/analyzer/atomic-builtins-1.c
>> create mode 100644
>> gcc/testsuite/gcc.dg/analyzer/atomic-builtins-haproxy-proxy.c
>> create mode 100644
>> gcc/testsuite/gcc.dg/analyzer/atomic-builtins-qemu-sockets.c
>> create mode 100644 gcc/testsuite/gcc.dg/analyzer/atomic-types-1.c
>>
>> diff --git a/gcc/analyzer/kf.cc b/gcc/analyzer/kf.cc
>> index 93c46630f36..104499e 100644
>> --- a/gcc/analyzer/kf.cc
>> +++ b/gcc/analyzer/kf.cc
>> @@ -69,6 +69,235 @@ kf_alloca::impl_call_pre (const call_details ) const
>> cd.maybe_set_lhs (ptr_sval);
>> }
>>
>> +/* Handler for:
>> + void __atomic_exchange (type *ptr, type *val, type *ret, int memorder).
>> */
>> +
>> +class kf_atomic_exchange : public internal_known_function
>> +{
>> +public:
>> + /* This is effectively:
>> + *RET = *PTR;
>> + *PTR = *VAL;
>> + */
>> + void impl_call_pre (const call_details ) const final override
>> + {
>> +const svalue *ptr_ptr_sval = cd.get_arg_svalue (0);
>> +tree ptr_ptr_tree = cd.get_arg_tree (0);
>> +const svalue *val_ptr_sval = cd.get_arg_svalue (1);
>> +tree val_ptr_tree = cd.get_arg_tree (1);
>> +const svalue *ret_ptr_sval = cd.get_arg_svalue (2);
>> +tree ret_ptr_tree = cd.get_arg_tree (2);
>> +/* Ignore the memorder param. */
>> +
>> +region_model *model = cd.get_model ();
>> +region_model_context *ctxt = cd.get_ctxt ();
>> +
>> +const region *val_region
>> + = model->deref_rvalue (val_ptr_sval, val_ptr_tree, ctxt);
>> +const svalue *star_val_sval = model->get_store_value (val_region, ctxt);
>> +const region *ptr_region
>> + = model->deref_rvalue (ptr_ptr_sval, ptr_ptr_tree, ctxt);
>> +const svalue *star_ptr_sval = model->get_store_value (ptr_region, ctxt);
>> +const region *ret_region
>> + = model->deref_rvalue (ret_ptr_sval, ret_ptr_tree, ctxt);
>> +model->set_value (ptr_region, star_val_sval, ctxt);
>> +model->set_value (re
Re: [pushed] analyzer: implement various atomic builtins [PR109015]
Hi David,
The new test ICEs the compiler on aarch64-linux-gnu [1]. Would you please
investigate?
Running gcc:gcc.dg/analyzer/analyzer.exp ...
FAIL: gcc.dg/analyzer/atomic-builtins-qemu-sockets.c (internal compiler error:
in validate, at analyzer/store.cc:1329)
FAIL: gcc.dg/analyzer/atomic-builtins-qemu-sockets.c (test for excess errors)
This is a simple native build on aarch64-linux-gnu. Please let me know if you
need any help in reproducing this.
[1]
https://ci.linaro.org/job/tcwg_gcc_check--master-aarch64-build/82/artifact/artifacts/artifacts.precommit/06-check_regression/results.compare/*view*/
Thanks!
--
Maxim Kuvyrkov
https://www.linaro.org
> On Jun 2, 2023, at 17:32, David Malcolm via Gcc-patches
> wrote:
>
> This patch implements many of the __atomic_* builtins from
> sync-builtins.def as known_function subclasses within the analyzer.
>
> Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu.
> Pushed to trunk as r14-1497-gef768035ae8090.
>
> gcc/analyzer/ChangeLog:
> PR analyzer/109015
> * kf.cc (class kf_atomic_exchange): New.
> (class kf_atomic_exchange_n): New.
> (class kf_atomic_fetch_op): New.
> (class kf_atomic_op_fetch): New.
> (class kf_atomic_load): New.
> (class kf_atomic_load_n): New.
> (class kf_atomic_store_n): New.
> (register_atomic_builtins): New function.
> (register_known_functions): Call register_atomic_builtins.
>
> gcc/testsuite/ChangeLog:
> PR analyzer/109015
> * gcc.dg/analyzer/atomic-builtins-1.c: New test.
> * gcc.dg/analyzer/atomic-builtins-haproxy-proxy.c: New test.
> * gcc.dg/analyzer/atomic-builtins-qemu-sockets.c: New test.
> * gcc.dg/analyzer/atomic-types-1.c: New test.
> ---
> gcc/analyzer/kf.cc| 355
> .../gcc.dg/analyzer/atomic-builtins-1.c | 544 ++
> .../analyzer/atomic-builtins-haproxy-proxy.c | 55 ++
> .../analyzer/atomic-builtins-qemu-sockets.c | 18 +
> .../gcc.dg/analyzer/atomic-types-1.c | 11 +
> 5 files changed, 983 insertions(+)
> create mode 100644 gcc/testsuite/gcc.dg/analyzer/atomic-builtins-1.c
> create mode 100644
> gcc/testsuite/gcc.dg/analyzer/atomic-builtins-haproxy-proxy.c
> create mode 100644
> gcc/testsuite/gcc.dg/analyzer/atomic-builtins-qemu-sockets.c
> create mode 100644 gcc/testsuite/gcc.dg/analyzer/atomic-types-1.c
>
> diff --git a/gcc/analyzer/kf.cc b/gcc/analyzer/kf.cc
> index 93c46630f36..104499e 100644
> --- a/gcc/analyzer/kf.cc
> +++ b/gcc/analyzer/kf.cc
> @@ -69,6 +69,235 @@ kf_alloca::impl_call_pre (const call_details ) const
> cd.maybe_set_lhs (ptr_sval);
> }
>
> +/* Handler for:
> + void __atomic_exchange (type *ptr, type *val, type *ret, int memorder).
> */
> +
> +class kf_atomic_exchange : public internal_known_function
> +{
> +public:
> + /* This is effectively:
> + *RET = *PTR;
> + *PTR = *VAL;
> + */
> + void impl_call_pre (const call_details ) const final override
> + {
> +const svalue *ptr_ptr_sval = cd.get_arg_svalue (0);
> +tree ptr_ptr_tree = cd.get_arg_tree (0);
> +const svalue *val_ptr_sval = cd.get_arg_svalue (1);
> +tree val_ptr_tree = cd.get_arg_tree (1);
> +const svalue *ret_ptr_sval = cd.get_arg_svalue (2);
> +tree ret_ptr_tree = cd.get_arg_tree (2);
> +/* Ignore the memorder param. */
> +
> +region_model *model = cd.get_model ();
> +region_model_context *ctxt = cd.get_ctxt ();
> +
> +const region *val_region
> + = model->deref_rvalue (val_ptr_sval, val_ptr_tree, ctxt);
> +const svalue *star_val_sval = model->get_store_value (val_region, ctxt);
> +const region *ptr_region
> + = model->deref_rvalue (ptr_ptr_sval, ptr_ptr_tree, ctxt);
> +const svalue *star_ptr_sval = model->get_store_value (ptr_region, ctxt);
> +const region *ret_region
> + = model->deref_rvalue (ret_ptr_sval, ret_ptr_tree, ctxt);
> +model->set_value (ptr_region, star_val_sval, ctxt);
> +model->set_value (ret_region, star_ptr_sval, ctxt);
> + }
> +};
> +
> +/* Handler for:
> + __atomic_exchange_n (type *ptr, type val, int memorder). */
> +
> +class kf_atomic_exchange_n : public internal_known_function
> +{
> +public:
> + /* This is effectively:
> + RET = *PTR;
> + *PTR = VAL;
> + return RET;
> + */
> + void impl_call_pre (const call_details ) const final override
> + {
> +const svalue *ptr_sval = cd.get_arg_svalue (0);
> +tree ptr_tree = cd.get_arg_tree (0);
> +const svalue *set_sval = cd.get_arg_svalue (1);
> +/* Ignore the memorder param. */
> +
> +region_model *model = cd.get_model ();
> +region_model_context *ctxt = cd.get_ctxt ();
> +
>
[PATCH 12/12] [contrib] validate_failures.py: Ignore stray filesystem paths in results
This patch simplifies comparison of results that have filesystem
paths. E.g., (assuming different values of ):
Running
/home/user/gcc-N/gcc/testsuite/gcc.target/aarch64/sve/acle/aarch64-sve-acle-asm.exp
...
ERROR: tcl error sourcing
/home/user/gcc-N/gcc/testsuite/gcc.target/aarch64/sve/acle/aarch64-sve-acle-asm.exp.
We add "--srcpath ", option, and set it by default to
"[^ ]+/testsuite/", which works well for all components of the GNU
Toolchain. We then remove substrings matching from paths of
.exp files and from occasional "ERROR:" results.
---
contrib/testsuite-management/validate_failures.py | 15 +++
1 file changed, 15 insertions(+)
diff --git a/contrib/testsuite-management/validate_failures.py
b/contrib/testsuite-management/validate_failures.py
index a77aabe0bdd..4dfd9cda4e2 100755
--- a/contrib/testsuite-management/validate_failures.py
+++ b/contrib/testsuite-management/validate_failures.py
@@ -135,6 +135,9 @@ class TestResult(object):
(self.state,
self.name,
self.description) =
_VALID_TEST_RESULTS_REX.match(summary_line).groups()
+if _OPTIONS.srcpath_regex and _OPTIONS.srcpath_regex != '':
+ self.description = re.sub(_OPTIONS.srcpath_regex, '',
+self.description)
except:
print('Failed to parse summary line: "%s"' % summary_line,
file=sys.stderr)
@@ -361,6 +364,9 @@ def ParseManifestWorker(result_set, manifest_path):
result_set.add(result)
elif IsExpLine(orig_line):
result_set.current_exp = _EXP_LINE_REX.match(orig_line).groups()[0]
+ if _OPTIONS.srcpath_regex and _OPTIONS.srcpath_regex != '':
+result_set.current_exp = re.sub(_OPTIONS.srcpath_regex, '',
+result_set.current_exp)
elif IsToolLine(orig_line):
result_set.current_tool = _TOOL_LINE_REX.match(orig_line).groups()[0]
elif IsSummaryLine(orig_line):
@@ -400,6 +406,9 @@ def ParseSummary(sum_fname):
result_set.add(result)
elif IsExpLine(line):
result_set.current_exp = _EXP_LINE_REX.match(line).groups()[0]
+ if _OPTIONS.srcpath_regex and _OPTIONS.srcpath_regex != '':
+result_set.current_exp = re.sub(_OPTIONS.srcpath_regex, '',
+result_set.current_exp)
result_set.testsuites.add((result_set.current_tool,
result_set.current_exp))
elif IsToolLine(line):
@@ -640,6 +649,12 @@ def Main(argv):
help='Use provided date MMDD to decide whether '
'manifest entries with expiry settings have expired '
'or not. (default = Use today date)')
+ parser.add_option('--srcpath', action='store', type='string',
+dest='srcpath_regex', default='[^ ]+/testsuite/',
+help='Remove provided path (can be a regex) from '
+'the result entries. This is useful to remove '
+'occasional filesystem path from the results. '
+'(default = "[^ ]+/testsuite/")')
parser.add_option('--inverse_match', action='store_true',
dest='inverse_match', default=False,
help='Inverse result sets in comparison. '
--
2.34.1
[PATCH 08/12] [contrib] validate_failures.py: Support "$tool:" prefix in exp names
From: Christophe Lyon
This makes it easier to extract the $tool:$exp pair when iterating
over failures/flaky tests, which, in turn, simplifies re-running
testsuite parts that have unexpected failures or passes.
---
contrib/testsuite-management/validate_failures.py | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/contrib/testsuite-management/validate_failures.py
b/contrib/testsuite-management/validate_failures.py
index c4b9fc377ce..6dcdcf5c69b 100755
--- a/contrib/testsuite-management/validate_failures.py
+++ b/contrib/testsuite-management/validate_failures.py
@@ -67,12 +67,14 @@ _VALID_TEST_RESULTS_REX = re.compile('(%s):\s*(\S+)\s*(.*)'
# Formats of .sum file sections
_TOOL_LINE_FORMAT = '\t\t=== %s tests ===\n'
-_EXP_LINE_FORMAT = '\nRunning %s ...\n'
+_EXP_LINE_FORMAT = '\nRunning %s:%s ...\n'
_SUMMARY_LINE_FORMAT = '\n\t\t=== %s Summary ===\n'
# ... and their compiled regexs.
_TOOL_LINE_REX = re.compile('^\t\t=== (.*) tests ===\n')
-_EXP_LINE_REX = re.compile('^Running (.*\.exp) \.\.\.\n')
+# Match .exp file name, optionally prefixed by a "tool:" name and a
+# path ending with "testsuite/"
+_EXP_LINE_REX = re.compile('^Running (?:.*:)?(.*) \.\.\.\n')
_SUMMARY_LINE_REX = re.compile('^\t\t=== (.*) Summary ===\n')
# Subdirectory of srcdir in which to find the manifest file.
@@ -236,7 +238,7 @@ class ResultSet(set):
outfile.write(_TOOL_LINE_FORMAT % current_tool)
if current_exp != result.exp:
current_exp = result.exp
-outfile.write(_EXP_LINE_FORMAT % current_exp)
+outfile.write(_EXP_LINE_FORMAT % (current_tool, current_exp))
outfile.write('%s\n' % result)
outfile.write(_SUMMARY_LINE_FORMAT % 'Results')
--
2.34.1
[PATCH 11/12] [contrib] validate_failures.py: Add "--expiry_date YYYYMMDD" option
This option sets "today" date to compare expiration entries against.
Setting expiration date into the future allows re-detection of flaky
tests and creating fresh entries for them before the current flaky
entries expire.
---
.../testsuite-management/validate_failures.py | 24 +--
1 file changed, 22 insertions(+), 2 deletions(-)
diff --git a/contrib/testsuite-management/validate_failures.py
b/contrib/testsuite-management/validate_failures.py
index 6eb1acd473f..a77aabe0bdd 100755
--- a/contrib/testsuite-management/validate_failures.py
+++ b/contrib/testsuite-management/validate_failures.py
@@ -206,8 +206,7 @@ class TestResult(object):
# Return True if the expiration date of this result has passed.
expiration_date = self.ExpirationDate()
if expiration_date:
- now = datetime.date.today()
- return now > expiration_date
+ return _OPTIONS.expiry_today_date > expiration_date
class ResultSet(set):
@@ -636,6 +635,11 @@ def Main(argv):
default=False, help='When used with --produce_manifest, '
'it will overwrite an existing manifest file '
'(default = False)')
+ parser.add_option('--expiry_date', action='store',
+dest='expiry_today_date', default=None,
+help='Use provided date MMDD to decide whether '
+'manifest entries with expiry settings have expired '
+'or not. (default = Use today date)')
parser.add_option('--inverse_match', action='store_true',
dest='inverse_match', default=False,
help='Inverse result sets in comparison. '
@@ -670,6 +674,22 @@ def Main(argv):
global _OPTIONS
(_OPTIONS, _) = parser.parse_args(argv[1:])
+ # Set "today" date to compare expiration entries against.
+ # Setting expiration date into the future allows re-detection of flaky
+ # tests and creating fresh entries for them before the current flaky entries
+ # expire.
+ if _OPTIONS.expiry_today_date:
+today_date = re.search(r'(\d\d\d\d)(\d\d)(\d\d)',
+ _OPTIONS.expiry_today_date)
+if not today_date:
+Error('Invalid --expiry_today_date format "%s". Must be of the form '
+ '"expire=MMDD"' % _OPTIONS.expiry_today_date)
+_OPTIONS.expiry_today_date=datetime.date(int(today_date.group(1)),
+ int(today_date.group(2)),
+ int(today_date.group(3)))
+ else:
+_OPTIONS.expiry_today_date = datetime.date.today()
+
if _OPTIONS.produce_manifest:
retval = ProduceManifest()
elif _OPTIONS.clean_build:
--
2.34.1
[PATCH 09/12] [contrib] validate_failures.py: Improve error output
From: Thiago Bauermann
- Print message in case of broken sum file error.
- Print error messages to stderr. The script's stdout is, usually,
redirected to a file, and error messages shouldn't go there.
---
contrib/testsuite-management/validate_failures.py | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/contrib/testsuite-management/validate_failures.py
b/contrib/testsuite-management/validate_failures.py
index 6dcdcf5c69b..1919935cf53 100755
--- a/contrib/testsuite-management/validate_failures.py
+++ b/contrib/testsuite-management/validate_failures.py
@@ -136,12 +136,15 @@ class TestResult(object):
self.name,
self.description) =
_VALID_TEST_RESULTS_REX.match(summary_line).groups()
except:
-print('Failed to parse summary line: "%s"' % summary_line)
+print('Failed to parse summary line: "%s"' % summary_line,
+ file=sys.stderr)
raise
self.ordinal = ordinal
if tool == None or exp == None:
# .sum file seem to be broken. There was no "tool" and/or "exp"
# lines preceding this result.
+print(f'.sum file seems to be broken: tool="{tool}", exp="{exp}",
summary_line="{summary_line}"',
+ file=sys.stderr)
raise
self.tool = tool
self.exp = exp
--
2.34.1
[PATCH 07/12] [contrib] validate_failures.py: Use exit code "2" to indicate regression
... in the results. Python exits with code "1" on exceptions and internal errors, which we use to detect failure to parse results. --- contrib/testsuite-management/validate_failures.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contrib/testsuite-management/validate_failures.py b/contrib/testsuite-management/validate_failures.py index f2d7b099d78..c4b9fc377ce 100755 --- a/contrib/testsuite-management/validate_failures.py +++ b/contrib/testsuite-management/validate_failures.py @@ -645,7 +645,7 @@ def Main(argv): if retval: return 0 else: -return 1 +return 2 if __name__ == '__main__': -- 2.34.1
[PATCH 05/12] [contrib] validate_failures.py: Add more verbosity levels
... to control validate_failures.py output
---
.../testsuite-management/validate_failures.py | 82 +++
1 file changed, 46 insertions(+), 36 deletions(-)
diff --git a/contrib/testsuite-management/validate_failures.py
b/contrib/testsuite-management/validate_failures.py
index 1bd09e0c20c..26ea1d6f53b 100755
--- a/contrib/testsuite-management/validate_failures.py
+++ b/contrib/testsuite-management/validate_failures.py
@@ -324,7 +324,7 @@ def GetNegativeResult(line):
def ParseManifestWorker(result_set, manifest_path):
"""Read manifest_path, adding the contents to result_set."""
- if _OPTIONS.verbosity >= 1:
+ if _OPTIONS.verbosity >= 5:
print('Parsing manifest file %s.' % manifest_path)
manifest_file = open(manifest_path, encoding='latin-1', mode='r')
for orig_line in manifest_file:
@@ -380,7 +380,8 @@ def ParseSummary(sum_fname):
# Tests that have expired are not added to the set of expected
# results. If they are still present in the set of actual results,
# they will cause an error to be reported.
-print('WARNING: Expected failure "%s" has expired.' % line.strip())
+if _OPTIONS.verbosity >= 4:
+ print('WARNING: Expected failure "%s" has expired.' % line.strip())
continue
result_set.add(result)
elif IsExpLine(line):
@@ -425,7 +426,8 @@ def GetResults(sum_files, build_results = None):
if build_results == None:
build_results = ResultSet()
for sum_fname in sum_files:
-print('\t%s' % sum_fname)
+if _OPTIONS.verbosity >= 3:
+ print('\t%s' % sum_fname)
build_results |= ParseSummary(sum_fname)
return build_results
@@ -489,42 +491,46 @@ def GetBuildData():
return None, None
srcdir = GetMakefileValue('%s/Makefile' % _OPTIONS.build_dir, 'srcdir =')
target = GetMakefileValue('%s/Makefile' % _OPTIONS.build_dir,
'target_alias=')
- print('Source directory: %s' % srcdir)
- print('Build target: %s' % target)
+ if _OPTIONS.verbosity >= 3:
+print('Source directory: %s' % srcdir)
+print('Build target: %s' % target)
return srcdir, target
-def PrintSummary(msg, summary):
- print('\n\n%s' % msg)
+def PrintSummary(summary):
summary.Print()
def GetSumFiles(results, build_dir):
if not results:
-print('Getting actual results from build directory %s' % build_dir)
+if _OPTIONS.verbosity >= 3:
+ print('Getting actual results from build directory %s' % build_dir)
sum_files = CollectSumFiles(build_dir)
else:
-print('Getting actual results from user-provided results')
+if _OPTIONS.verbosity >= 3:
+ print('Getting actual results from user-provided results')
sum_files = results.split()
return sum_files
-def PerformComparison(expected, actual, ignore_missing_failures):
+def PerformComparison(expected, actual):
actual_vs_expected, expected_vs_actual = CompareResults(expected, actual)
tests_ok = True
if len(actual_vs_expected) > 0:
-PrintSummary('Unexpected results in this build (new failures)',
- actual_vs_expected)
+if _OPTIONS.verbosity >= 3:
+ print('\n\nUnexpected results in this build (new failures)')
+if _OPTIONS.verbosity >= 1:
+ PrintSummary(actual_vs_expected)
tests_ok = False
- if not ignore_missing_failures and len(expected_vs_actual) > 0:
-PrintSummary('Expected results not present in this build (fixed tests)'
- '\n\nNOTE: This is not a failure. It just means that these '
- 'tests were expected\nto fail, but either they worked in '
- 'this configuration or they were not\npresent at all.\n',
- expected_vs_actual)
+ if _OPTIONS.verbosity >= 2 and len(expected_vs_actual) > 0:
+print('\n\nExpected results not present in this build (fixed tests)'
+ '\n\nNOTE: This is not a failure. It just means that these '
+ 'tests were expected\nto fail, but either they worked in '
+ 'this configuration or they were not\npresent at all.\n')
+PrintSummary(expected_vs_actual)
- if tests_ok:
+ if tests_ok and _OPTIONS.verbosity >= 3:
print('\nSUCCESS: No unexpected failures.')
return tests_ok
@@ -532,21 +538,25 @@ def PerformComparison(expected, actual,
ignore_missing_failures):
def CheckExpectedResults():
manifest_path = GetManifestPath(True)
- print('Manifest: %s' % manifest_path)
+ if _OPTIONS.verbosity >= 3:
+print('Manifest: %s' % manifest_path)
manifest = GetManifest(manifest_path)
sum_files = GetSumFiles(_OPTIONS.results, _OPTIONS.build_dir)
actual = GetResults(sum_files)
- if _OPTIONS.verbosity >= 1:
-PrintSummary('Tests expected to fail', manifest)
-PrintSummary('\nActual test results', actual)
+ if _OPTIONS.verbosity >= 5:
+print('\n\nTests expected to fail')
+PrintSummary(manifest)
+print('\n\nActual test results')
+PrintSummary(actual)
- return
[PATCH 06/12] [contrib] validate_failures.py: Be more stringent in parsing result lines
Before this patch we would identify malformed line
"UNRESOLVEDTest run by tcwg-buildslave on Mon Aug 23 10:17:50 2021"
as an interesting result, only to fail in TestResult:__init__ due
to missing ":" after UNRESOLVED.
This patch makes all places that parse result lines use a single
compiled regex.
---
contrib/testsuite-management/validate_failures.py | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/contrib/testsuite-management/validate_failures.py
b/contrib/testsuite-management/validate_failures.py
index 26ea1d6f53b..f2d7b099d78 100755
--- a/contrib/testsuite-management/validate_failures.py
+++ b/contrib/testsuite-management/validate_failures.py
@@ -60,9 +60,10 @@ import os
import re
import sys
-# Handled test results.
_VALID_TEST_RESULTS = [ 'FAIL', 'UNRESOLVED', 'XPASS', 'ERROR' ]
-_VALID_TEST_RESULTS_REX = re.compile("%s" % "|".join(_VALID_TEST_RESULTS))
+# :
[PATCH 02/12] [contrib] validate_failures.py: Support expiry attributes in manifests
---
contrib/testsuite-management/validate_failures.py | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/contrib/testsuite-management/validate_failures.py
b/contrib/testsuite-management/validate_failures.py
index 94ba2e58b51..7351ba120b7 100755
--- a/contrib/testsuite-management/validate_failures.py
+++ b/contrib/testsuite-management/validate_failures.py
@@ -338,7 +338,13 @@ def ParseManifestWorker(result_set, manifest_path):
elif IsInclude(line):
ParseManifestWorker(result_set, GetIncludeFile(line, manifest_path))
elif IsInterestingResult(line):
- result_set.add(result_set.MakeTestResult(line))
+ result = result_set.MakeTestResult(line)
+ if result.HasExpired():
+# Ignore expired manifest entries.
+if _OPTIONS.verbosity >= 4:
+ print('WARNING: Expected failure "%s" has expired.' % line.strip())
+continue
+ result_set.add(result)
elif IsExpLine(orig_line):
result_set.current_exp = _EXP_LINE_REX.match(orig_line).groups()[0]
elif IsToolLine(orig_line):
@@ -369,6 +375,8 @@ def ParseSummary(sum_fname):
result = result_set.MakeTestResult(line, ordinal)
ordinal += 1
if result.HasExpired():
+# ??? What is the use-case for this? How "expiry" annotations are
+# ??? supposed to be added to .sum results?
# Tests that have expired are not added to the set of expected
# results. If they are still present in the set of actual results,
# they will cause an error to be reported.
--
2.34.1
[PATCH 03/12] [contrib] validate_failures.py: Read in manifest when comparing build dirs
This allows comparison of two build directories with a manifest
listing known flaky tests on the side.
---
contrib/testsuite-management/validate_failures.py | 14 +++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/contrib/testsuite-management/validate_failures.py
b/contrib/testsuite-management/validate_failures.py
index 7351ba120b7..4733dd89dcb 100755
--- a/contrib/testsuite-management/validate_failures.py
+++ b/contrib/testsuite-management/validate_failures.py
@@ -420,9 +420,10 @@ def CollectSumFiles(builddir):
return sum_files
-def GetResults(sum_files):
+def GetResults(sum_files, build_results = None):
"""Collect all the test results from the given .sum files."""
- build_results = ResultSet()
+ if build_results == None:
+build_results = ResultSet()
for sum_fname in sum_files:
print('\t%s' % sum_fname)
build_results |= ParseSummary(sum_fname)
@@ -567,8 +568,15 @@ def CompareBuilds():
sum_files = GetSumFiles(_OPTIONS.results, _OPTIONS.build_dir)
actual = GetResults(sum_files)
+ clean = ResultSet()
+
+ if _OPTIONS.manifest:
+manifest_path = GetManifestPath(srcdir, target, True)
+print('Manifest: %s' % manifest_path)
+clean = GetManifest(manifest_path)
+
clean_sum_files = GetSumFiles(_OPTIONS.results, _OPTIONS.clean_build)
- clean = GetResults(clean_sum_files)
+ clean = GetResults(clean_sum_files, clean)
return PerformComparison(clean, actual, _OPTIONS.ignore_missing_failures)
--
2.34.1
[PATCH 10/12] [contrib] validate_failures.py: Add new option --invert_match
This option is used to detect flaky tests that FAILed in the clean
build (or manifest), but PASSed in the current build (or manifest).
The option inverts output logic similar to what "-v/--invert-match"
does for grep.
---
.../testsuite-management/validate_failures.py | 34 +--
1 file changed, 32 insertions(+), 2 deletions(-)
diff --git a/contrib/testsuite-management/validate_failures.py
b/contrib/testsuite-management/validate_failures.py
index 1919935cf53..6eb1acd473f 100755
--- a/contrib/testsuite-management/validate_failures.py
+++ b/contrib/testsuite-management/validate_failures.py
@@ -217,11 +217,17 @@ class ResultSet(set):
Attributes:
current_tool: Name of the current top-level DejaGnu testsuite.
current_exp: Name of the current .exp testsuite file.
+testsuites: A set of (tool, exp) tuples representing encountered
testsuites.
"""
def __init__(self):
super().__init__()
self.ResetToolExp()
+self.testsuites=set()
+
+ def update(self, other):
+super().update(other)
+self.testsuites.update(other.testsuites)
def ResetToolExp(self):
self.current_tool = None
@@ -246,6 +252,10 @@ class ResultSet(set):
outfile.write(_SUMMARY_LINE_FORMAT % 'Results')
+ # Check if testsuite of expected_result is present in current results.
+ # This is used to compare partial test results against a full manifest.
+ def HasTestsuite(self, expected_result):
+return (expected_result.tool, expected_result.exp) in self.testsuites
def GetMakefileValue(makefile_name, value_name):
if os.path.exists(makefile_name):
@@ -391,6 +401,8 @@ def ParseSummary(sum_fname):
result_set.add(result)
elif IsExpLine(line):
result_set.current_exp = _EXP_LINE_REX.match(line).groups()[0]
+ result_set.testsuites.add((result_set.current_tool,
+ result_set.current_exp))
elif IsToolLine(line):
result_set.current_tool = _TOOL_LINE_REX.match(line).groups()[0]
result_set.current_exp = None
@@ -433,7 +445,7 @@ def GetResults(sum_files, build_results = None):
for sum_fname in sum_files:
if _OPTIONS.verbosity >= 3:
print('\t%s' % sum_fname)
-build_results |= ParseSummary(sum_fname)
+build_results.update(ParseSummary(sum_fname))
return build_results
@@ -458,7 +470,11 @@ def CompareResults(manifest, actual):
# Ignore tests marked flaky.
if 'flaky' in expected_result.attrs:
continue
-if expected_result not in actual:
+# We try to support comparing partial results vs full manifest
+# (e.g., manifest has failures for gcc, g++, gfortran, but we ran only
+# g++ testsuite). To achieve this we record encountered testsuites in
+# actual.testsuites set, and then we check it here using HasTestsuite().
+if expected_result not in actual and actual.HasTestsuite(expected_result):
manifest_vs_actual.add(expected_result)
return actual_vs_manifest, manifest_vs_actual
@@ -520,6 +536,13 @@ def GetSumFiles(results, build_dir):
def PerformComparison(expected, actual):
actual_vs_expected, expected_vs_actual = CompareResults(expected, actual)
+ if _OPTIONS.inverse_match:
+# Switch results if inverse comparison is requested.
+# This is useful in detecting flaky tests that FAILed in expected set,
+# but PASSed in actual set.
+actual_vs_expected, expected_vs_actual \
+ = expected_vs_actual, actual_vs_expected
+
tests_ok = True
if len(actual_vs_expected) > 0:
if _OPTIONS.verbosity >= 3:
@@ -613,6 +636,13 @@ def Main(argv):
default=False, help='When used with --produce_manifest, '
'it will overwrite an existing manifest file '
'(default = False)')
+ parser.add_option('--inverse_match', action='store_true',
+dest='inverse_match', default=False,
+help='Inverse result sets in comparison. '
+'Output unexpected passes as unexpected failures and '
+'unexpected failures as unexpected passes. '
+'This is used to catch FAIL->PASS flaky tests. '
+'(default = False)')
parser.add_option('--manifest', action='store', type='string',
dest='manifest', default=None,
help='Name of the manifest file to use (default = '
--
2.34.1
[PATCH 04/12] [contrib] validate_failures.py: Simplify GetManifestPath()
... and don't require a valid build directory when no data from it
is necessary.
---
contrib/testsuite-management/validate_failures.py | 13 +
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/contrib/testsuite-management/validate_failures.py
b/contrib/testsuite-management/validate_failures.py
index 4733dd89dcb..1bd09e0c20c 100755
--- a/contrib/testsuite-management/validate_failures.py
+++ b/contrib/testsuite-management/validate_failures.py
@@ -457,7 +457,7 @@ def CompareResults(manifest, actual):
return actual_vs_manifest, manifest_vs_actual
-def GetManifestPath(srcdir, target, user_provided_must_exist):
+def GetManifestPath(user_provided_must_exist):
"""Return the full path to the manifest file."""
manifest_path = _OPTIONS.manifest
if manifest_path:
@@ -465,6 +465,7 @@ def GetManifestPath(srcdir, target,
user_provided_must_exist):
Error('Manifest does not exist: %s' % manifest_path)
return manifest_path
else:
+(srcdir, target) = GetBuildData()
if not srcdir:
Error('Could not determine the location of GCC\'s source tree. '
'The Makefile does not contain a definition for "srcdir".')
@@ -530,8 +531,7 @@ def PerformComparison(expected, actual,
ignore_missing_failures):
def CheckExpectedResults():
- srcdir, target = GetBuildData()
- manifest_path = GetManifestPath(srcdir, target, True)
+ manifest_path = GetManifestPath(True)
print('Manifest: %s' % manifest_path)
manifest = GetManifest(manifest_path)
sum_files = GetSumFiles(_OPTIONS.results, _OPTIONS.build_dir)
@@ -545,8 +545,7 @@ def CheckExpectedResults():
def ProduceManifest():
- (srcdir, target) = GetBuildData()
- manifest_path = GetManifestPath(srcdir, target, False)
+ manifest_path = GetManifestPath(False)
print('Manifest: %s' % manifest_path)
if os.path.exists(manifest_path) and not _OPTIONS.force:
Error('Manifest file %s already exists.\nUse --force to overwrite.' %
@@ -563,15 +562,13 @@ def ProduceManifest():
def CompareBuilds():
- (srcdir, target) = GetBuildData()
-
sum_files = GetSumFiles(_OPTIONS.results, _OPTIONS.build_dir)
actual = GetResults(sum_files)
clean = ResultSet()
if _OPTIONS.manifest:
-manifest_path = GetManifestPath(srcdir, target, True)
+manifest_path = GetManifestPath(True)
print('Manifest: %s' % manifest_path)
clean = GetManifest(manifest_path)
--
2.34.1
[PATCH 01/12] [contrib] validate_failures.py: Avoid testsuite aliasing
This patch adds tracking of current testsuite "tool" and "exp"
to the processing of .sum files. This avoids aliasing between
tests from different testsuites with same name+description.
E.g., this is necessary for testsuite/c-c++-common, which is ran
for both gcc and g++ "tools".
This patch changes manifest format from ...
FAIL: gcc_test
FAIL: g++_test
... to ...
=== gcc tests ===
Running gcc/foo.exp ...
FAIL: gcc_test
=== gcc Summary ==
=== g++ tests ===
Running g++/bar.exp ...
FAIL: g++_test
=== g++ Summary ==
.
The new format uses same formatting as DejaGnu's .sum files
to specify which "tool" and "exp" the test belongs to.
---
.../testsuite-management/validate_failures.py | 137 +++---
1 file changed, 115 insertions(+), 22 deletions(-)
diff --git a/contrib/testsuite-management/validate_failures.py
b/contrib/testsuite-management/validate_failures.py
index 43d9d50af8d..94ba2e58b51 100755
--- a/contrib/testsuite-management/validate_failures.py
+++ b/contrib/testsuite-management/validate_failures.py
@@ -64,6 +64,16 @@ import sys
_VALID_TEST_RESULTS = [ 'FAIL', 'UNRESOLVED', 'XPASS', 'ERROR' ]
_VALID_TEST_RESULTS_REX = re.compile("%s" % "|".join(_VALID_TEST_RESULTS))
+# Formats of .sum file sections
+_TOOL_LINE_FORMAT = '\t\t=== %s tests ===\n'
+_EXP_LINE_FORMAT = '\nRunning %s ...\n'
+_SUMMARY_LINE_FORMAT = '\n\t\t=== %s Summary ===\n'
+
+# ... and their compiled regexs.
+_TOOL_LINE_REX = re.compile('^\t\t=== (.*) tests ===\n')
+_EXP_LINE_REX = re.compile('^Running (.*\.exp) \.\.\.\n')
+_SUMMARY_LINE_REX = re.compile('^\t\t=== (.*) Summary ===\n')
+
# Subdirectory of srcdir in which to find the manifest file.
_MANIFEST_SUBDIR = 'contrib/testsuite-management'
@@ -111,9 +121,11 @@ class TestResult(object):
ordinal: Monotonically increasing integer.
It is used to keep results for one .exp file sorted
by the order the tests were run.
+tool: Top-level testsuite name (aka "tool" in DejaGnu parlance) of the
test.
+exp: Name of .exp testsuite file.
"""
- def __init__(self, summary_line, ordinal=-1):
+ def __init__(self, summary_line, ordinal, tool, exp):
try:
(self.attrs, summary_line) = SplitAttributesFromSummaryLine(summary_line)
try:
@@ -125,6 +137,12 @@ class TestResult(object):
print('Failed to parse summary line: "%s"' % summary_line)
raise
self.ordinal = ordinal
+ if tool == None or exp == None:
+# .sum file seem to be broken. There was no "tool" and/or "exp"
+# lines preceding this result.
+raise
+ self.tool = tool
+ self.exp = exp
except ValueError:
Error('Cannot parse summary line "%s"' % summary_line)
@@ -133,14 +151,27 @@ class TestResult(object):
self.state, summary_line, self))
def __lt__(self, other):
-return (self.name < other.name or
-(self.name == other.name and self.ordinal < other.ordinal))
+if (self.tool != other.tool):
+ return self.tool < other.tool
+if (self.exp != other.exp):
+ return self.exp < other.exp
+if (self.name != other.name):
+ return self.name < other.name
+return self.ordinal < other.ordinal
def __hash__(self):
-return hash(self.state) ^ hash(self.name) ^ hash(self.description)
-
+return (hash(self.state) ^ hash(self.tool) ^ hash(self.exp)
+^ hash(self.name) ^ hash(self.description))
+
+ # Note that we don't include "attrs" in this comparison. This means that
+ # result entries "FAIL: test" and "flaky | FAIL: test" are considered
+ # the same. Therefore the ResultSet will preserve only the first occurence.
+ # In practice this means that flaky entries should preceed expected fails
+ # entries.
def __eq__(self, other):
return (self.state == other.state and
+self.tool == other.tool and
+self.exp == other.exp and
self.name == other.name and
self.description == other.description)
@@ -174,6 +205,43 @@ class TestResult(object):
return now > expiration_date
+class ResultSet(set):
+ """Describes a set of DejaGNU test results.
+ This set can be read in from .sum files or emitted as a manifest.
+
+ Attributes:
+current_tool: Name of the current top-level DejaGnu testsuite.
+current_exp: Name of the current .exp testsuite file.
+ """
+
+ def __init__(self):
+super().__init__()
+self.ResetToolExp()
+
+ def ResetToolExp(self):
+self.current_tool = None
+self.current_exp = None
+
+ def MakeTestResult(self, summary_line, ordinal=-1):
+return TestResult(summary_line, ordinal,
+ self.current_tool, self.current_exp)
+
+ def Print(self, outfile=sys.stdout):
+current_tool = None
+current_exp = None
+
+for result in sorted(self):
+ if current_tool != result.tool:
+current_tool = result.tool
+
[contrib] Extend and improve validate_failures.py
This patch series extends and improves validate_failures.py script to provide a powerful tool to handle DejaGnu test results in automated CI environment. Linaro TCWG uses validate_failures.py to ... - compare test results without human oversight, - detect unexpected FAILs vs baseline, - detect unexpected PASSes vs baseline, - automatically detect flaky tests, - create lists of expected failures and flaky tests, see [1]. [1] https://ci.linaro.org/job/tcwg_gcc_check--master-arm-build/lastSuccessfulBuild/artifact/artifacts/sumfiles/xfails.xfail/*view*/
Re: [PATCH] Couple of debug dump improvements to scheduler (no code-gen changes)
Hi Jeff, I've forgotten to commit these patches when they were approved 2 years ago. They still apply cleanly to the current mainline and I've retested them (bootstrap+regtest) on aarch64-linux-gnu and arm-linux-gnueabihf with no regressions. I'll commit these shortly. Regards, On Fri, 30 Aug 2019 at 01:57, Jeff Law wrote: > On 8/29/19 9:44 AM, Maxim Kuvyrkov wrote: > > Hi, > > > > The first patch adds ranking statistics for autoprefetcher heuristic. > > > > The second one makes it easier to diff scheduler debug dumps by adding > more context lines for diff at clock increments. > > > > OK to commit? > OK for both. > jeff > -- Maxim Kuvyrkov www.linaro.org 0003-Improve-diff-ability-of-scheduler-logs.patch Description: Binary data 0002-Add-missing-entry-for-rank_for_schedule-stats.patch Description: Binary data
Re: [PR91598] Improve autoprefetcher heuristic in haifa-sched.c
Hi All, I've forgotten to commit this patch when it was approved 2 years ago. It still applies cleanly to the current mainline and I've retested it (bootstrap+regtest) on aarch64-linux-gnu and arm-linux-gnueabihf with no regressions. I'll commit this shortly. Regards, On Tue, 3 Sept 2019 at 19:55, Wilco Dijkstra wrote: > Hi Maxim, > > > > Autoprefetching heuristic is enabled only for cores that support it, > and isn't active for by default. > > > > It's enabled on most cores, including the default (generic). So we do > have to be > > careful that this doesn't regress any other benchmarks or do worse on > modern > > cores. > > I benchmarked your scheduler change on a few AArch64 machines, and it > either has > no effect or a positive effect on SPECFP with no major outliers (and only > minor > codesize differences). So I think your proposed patch is OK as is. > > Cheers, > Wilco > > -- Maxim Kuvyrkov www.linaro.org 0001-Improve-autoprefetcher-heuristic-partly-fix-regressi.patch Description: Binary data
Re: disable -Warray-bounds in libgo (PR 101374)
> On 9 Jul 2021, at 09:16, Richard Biener via Gcc-patches > wrote: > > On Thu, Jul 8, 2021 at 8:02 PM Martin Sebor via Gcc-patches > wrote: >> >> Hi Ian, >> >> Yesterday's enhancement to -Warray-bounds has exposed a couple of >> issues in libgo where the code writes into an invalid constant >> address that the warning is designed to flag. >> >> On the assumption that those invalid addresses are deliberate, >> the attached patch suppresses these instances by using #pragma >> GCC diagnostic but I don't think I'm supposed to commit it (at >> least Git won't let me). To avoid Go bootstrap failures please >> either apply the patch or otherwise suppress the warning (e.g., >> by using a volatile pointer temporary). > > Btw, I don't think we should diagnose things like > >*(int*)0x21 = 0x21; > > when somebody literally writes that he'll be just annoyed by diagnostics. And we have an assortment of similar cases in 32-bit ARM kernel-page helpers. At the moment building libatomic for arm-linux-gnueabihf fails with: === In function ‘select_test_and_set_8’, inlined from ‘select_test_and_set_8’ at /home/tcwg-buildslave/workspace/tcwg-dev-build/snapshots/gcc.git~master/libatomic/tas_n.c:115:1: /home/tcwg-buildslave/workspace/tcwg-dev-build/snapshots/gcc.git~master/libatomic/config/linux/arm/host-config.h:42:34: error: array subscript 0 is outside array bounds of ‘unsigned int[0]’ [-Werror=array-bounds] 42 | #define __kernel_helper_version (*(unsigned int *)0x0ffc) | ~^~~~ === In libatomic/config/linux/arm/host-config.h we have: === /* Kernel helper for 32-bit compare-and-exchange. */ typedef int (__kernel_cmpxchg_t) (UWORD oldval, UWORD newval, UWORD *ptr); #define __kernel_cmpxchg (*(__kernel_cmpxchg_t *) 0x0fc0) /* Kernel helper for 64-bit compare-and-exchange. */ typedef int (__kernel_cmpxchg64_t) (const U_8 * oldval, const U_8 * newval, U_8 *ptr); #define __kernel_cmpxchg64 (*(__kernel_cmpxchg64_t *) 0x0f60) /* Kernel helper for memory barrier. */ typedef void (__kernel_dmb_t) (void); #define __kernel_dmb (*(__kernel_dmb_t *) 0x0fa0) /* Kernel helper page version number. */ #define __kernel_helper_version (*(unsigned int *)0x0ffc) === -- Maxim Kuvyrkov https://www.linaro.org
Re: [r12-2132 Regression] FAIL: g++.dg/warn/Warray-bounds-20.C -std=gnu++98 note (test for warnings, line 55) on Linux/x86_64
> On 9 Jul 2021, at 02:35, sunil.k.pandey via Gcc-patches
> wrote:
>
> On Linux/x86_64,
>
> a110855667782dac7b674d3e328b253b3b3c919b is the first bad commit
> commit a110855667782dac7b674d3e328b253b3b3c919b
> Author: Martin Sebor
> Date: Wed Jul 7 14:05:25 2021 -0600
>
>Correct handling of variable offset minus constant in -Warray-bounds
> [PR100137]
>
> caused
Hi Martin,
I see these failing on aarch64-linux-gnu as well:
>
> FAIL: gcc.dg/Wstringop-overflow-47.c pr97027 (test for warnings, line 34)
> FAIL: gcc.dg/Wstringop-overflow-47.c pr97027 (test for warnings, line 37)
> FAIL: gcc.dg/Wstringop-overflow-47.c pr97027 (test for warnings, line 42)
FWIW, I don’t see these on aarch64-linux-gnu:
> FAIL: g++.dg/warn/Warray-bounds-20.C -std=gnu++14 note (test for warnings,
> line 38)
> FAIL: g++.dg/warn/Warray-bounds-20.C -std=gnu++14 note (test for warnings,
> line 55)
> FAIL: g++.dg/warn/Warray-bounds-20.C -std=gnu++17 note (test for warnings,
> line 38)
> FAIL: g++.dg/warn/Warray-bounds-20.C -std=gnu++17 note (test for warnings,
> line 55)
> FAIL: g++.dg/warn/Warray-bounds-20.C -std=gnu++2a note (test for warnings,
> line 38)
> FAIL: g++.dg/warn/Warray-bounds-20.C -std=gnu++2a note (test for warnings,
> line 55)
> FAIL: g++.dg/warn/Warray-bounds-20.C -std=gnu++98 note (test for warnings,
> line 38)
> FAIL: g++.dg/warn/Warray-bounds-20.C -std=gnu++98 note (test for warnings,
> line 55)
--
Maxim Kuvyrkov
https://www.linaro.org
>
> with GCC configured with
>
> ../../gcc/configure
> --prefix=/local/skpandey/gccwork/toolwork/gcc-bisect-master/master/r12-2132/usr
> --enable-clocale=gnu --with-system-zlib --with-demangler-in-ld
> --with-fpmath=sse --enable-languages=c,c++,fortran --enable-cet --without-isl
> --enable-libmpx x86_64-linux --disable-bootstrap
>
> To reproduce:
>
> $ cd {build_dir}/gcc && make check
> RUNTESTFLAGS="dg.exp=gcc.dg/Wstringop-overflow-47.c
> --target_board='unix{-m32\ -march=cascadelake}'"
> $ cd {build_dir}/gcc && make check
> RUNTESTFLAGS="dg.exp=gcc.dg/Wstringop-overflow-47.c
> --target_board='unix{-m64\ -march=cascadelake}'"
> $ cd {build_dir}/gcc && make check
> RUNTESTFLAGS="dg.exp=g++.dg/warn/Warray-bounds-20.C
> --target_board='unix{-m32}'"
> $ cd {build_dir}/gcc && make check
> RUNTESTFLAGS="dg.exp=g++.dg/warn/Warray-bounds-20.C
> --target_board='unix{-m32\ -march=cascadelake}'"
>
> (Please do not reply to this email, for question about this report, contact
> me at skpgkp2 at gmail dot com)
[PATCH] [contrib] Remove broken compareSumTests3 script
Hi Matthew, Hi Sebastian, This patch removes compareSumTests3, which appears to be broken. It tries to call compareSumFiles(), which is nowhere to be found and was never present in the GCC repo. OK to remove? Regards, -- Maxim Kuvyrkov https://www.linaro.org 0001-contrib-Remove-broken-compareSumTests3-script.patch Description: Binary data
Re: [PATCH] Add description of how testsuite parallelization works
> On Jul 2, 2021, at 5:44 PM, Christophe Lyon > wrote: > > > > On Fri, Jul 2, 2021 at 4:29 PM Jakub Jelinek via Gcc-patches > wrote: > On Fri, Jul 02, 2021 at 05:20:33PM +0300, Maxim Kuvyrkov wrote: > > Hi Jakub, > > > > Thanks for helping me on IRC with debugging testsuite problems. Does this > > write up look good? > > > Hi Maxim, > Thanks for adding this! > > There's a typo in your ChangeLog : > * lib/gcc-dg.exp: Add a comment. > should be lib/gcc-defs.exp > > but I think the new commit hooks will catch it. It did, and I fixed that. Thanks! -- Maxim Kuvyrkov https://www.linaro.org > > Christophe > > > LGTM, thanks. > > Jakub >
[PATCH] Add description of how testsuite parallelization works
Hi Jakub, Thanks for helping me on IRC with debugging testsuite problems. Does this write up look good? Regards, -- Maxim Kuvyrkov https://www.linaro.org 0001-Add-description-of-how-testsuite-parallelization-wor.patch Description: Binary data
Re: [PATCH] Avoid loading an undefined value in the ranger_cache constructor.
> On 16 Jun 2021, at 20:14, Andrew MacLeod wrote: > > On 6/16/21 5:41 AM, Maxim Kuvyrkov wrote: >> >>> + m_new_value_p = state; >>> + return ret; >>> } >>>// Dump the caches for basic block BB to file F. >> Thanks, >> >> -- >> Maxim Kuvyrkov >> https://www.linaro.org >> > Let me know if the problem is resolved. > > pushed as obvious. > Hi Andrew, All good, thanks! CI is back to green. -- Maxim Kuvyrkov https://www.linaro.org
Re: [PATCH] tree-optimization PR/101014 - Limit new value calculations to first order effects.
> On 15 Jun 2021, at 00:07, Andrew MacLeod via Gcc-patches
> wrote:
>
> As mentioned in the Text from the PR:
>
> "When a range is being calculated for an ssa-name, the propagation process
> often goes along back edges. These back edges sometime require other
> ssa-names which have not be processed yet. These are flagged as "poor values"
> and when propagation is done, we visit the list of poor values, calculate
> them, and see if that may result if a better range for the original ssa-name.
>
> The problem is that calculating these poor values may also spawn another set
> of requests since the block at the far end of the back edge has not been
> processed yet... its highly likely that some additional unprocessed ssa-names
> are used in the calculation of that name, but typically they do not affect
> the current range in a significant way.
>
> Thus we mostly we care about the first order effect only. It turns out to be
> very rare that a 2nd order effect on a back edge affects anything that we
> don't catch later.
>
> This patch turns off poor-value tagging when looking up the first order
> values, thus avoiding the 2nd order and beyond cascading effects.
>
> I haven't found a test case we miss yet because of this change, yet it
> probably resolves a number of the outstanding compilation problems in a
> significant way.
>
> I think this will probably apply to gcc 11 in some form as well, so I'll look
> at an equivalent patch for there."
>
>
> This patch simplifies the enable_new_value routines.. replacing the
> enable/disable with an enable with flag routine, which returns the previous
> value.This lets us change the mode and then set it back to what it was
> before. Seems better in general.
>
> Then disables new values for 2nd+ order effects. GCC11 patch forthcoming.
>
> Bootstraps on x86_64-pc-linux-gnu, no regressions. pushed.
>
> Andrew
Hi Andrew,
This causes bootstrap-with-ubsan failure on at least aarch64-linux-gnu, likely,
others:
# 00:42:32
/home/tcwg-buildslave/workspace/tcwg_gnu_0/abe/snapshots/gcc.git~master/gcc/gimple-range-cache.cc:757:8:
runtime error: load of value 48, which is not a valid value for type 'bool'
# 00:42:32
/home/tcwg-buildslave/workspace/tcwg_gnu_0/abe/snapshots/gcc.git~master/gcc/gimple-range-cache.cc:757:8:
runtime error: load of value 48, which is not a valid value for type 'bool'
# 00:42:32
/home/tcwg-buildslave/workspace/tcwg_gnu_0/abe/snapshots/gcc.git~master/gcc/gimple-range-cache.cc:757:8:
runtime error: load of value 32, which is not a valid value for type 'bool'
# 00:42:32
/home/tcwg-buildslave/workspace/tcwg_gnu_0/abe/snapshots/gcc.git~master/gcc/gimple-range-cache.cc:757:8:
runtime error: load of value 48, which is not a valid value for type 'bool'
# 00:42:32
/home/tcwg-buildslave/workspace/tcwg_gnu_0/abe/snapshots/gcc.git~master/gcc/gimple-range-cache.cc:757:8:
runtime error: load of value 32, which is not a valid value for type 'bool'
# 00:42:32
/home/tcwg-buildslave/workspace/tcwg_gnu_0/abe/snapshots/gcc.git~master/gcc/gimple-range-cache.cc:757:8:
runtime error: load of value 48, which is not a valid value for type 'bool'
# 00:42:32
/home/tcwg-buildslave/workspace/tcwg_gnu_0/abe/snapshots/gcc.git~master/gcc/gimple-range-cache.cc:757:8:
runtime error: load of value 32, which is not a valid value for type 'bool'
# 00:42:32
/home/tcwg-buildslave/workspace/tcwg_gnu_0/abe/snapshots/gcc.git~master/gcc/gimple-range-cache.cc:757:8:
runtime error: load of value 32, which is not a valid value for type 'bool'
>
> @@ -748,21 +748,15 @@ ranger_cache::dump (FILE *f)
>fprintf (f, "\n");
> }
>
> -// Allow the cache to flag and query new values when propagation is forced
> -// to use an unknown value.
> +// Allow or disallow the cache to flag and query new values when propagation
> +// is forced to use an unknown value. The previous state is returned.
>
> -void
> -ranger_cache::enable_new_values ()
> -{
> - m_new_value_p = true;
> -}
> -
> -// Disable new value querying.
> -
> -void
> -ranger_cache::disable_new_values ()
> +bool
> +ranger_cache::enable_new_values (bool state)
> {
> - m_new_value_p = false;
> + bool ret = m_new_value_p;
I think changing this to
bool ret = (bool) m_new_value_p;
might be enough, but you know this code better.
Would you please take a look at this?
> + m_new_value_p = state;
> + return ret;
> }
>
> // Dump the caches for basic block BB to file F.
Thanks,
--
Maxim Kuvyrkov
https://www.linaro.org
Re: [PATCH] middle-end/94188 fix fold of addr expression generation
> On 17 Mar 2020, at 17:40, Richard Biener wrote: > > > This adds a missing type conversion to build_fold_addr_expr and adjusts > fallout - build_fold_addr_expr was used as a convenience to build an > ADDR_EXPR but some callers do not expect the result to be simplified > to something else. > > Bootstrapped on x86_64-unknown-linux-gnu, testin in progress. > > This is the 3rd or 4th attempt and I hope to have catched all fallout > with this. I think it's inevitable we fix the mistake in > build_fold_addr_expr. > > Richard. > > 2020-03-17 Richard Biener > > PR middle-end/94188 > * fold-const.c (build_fold_addr_expr): Convert address to > correct type. > * asan.c (maybe_create_ssa_name): Strip useless type conversions. > * gimple-fold.c (gimple_fold_stmt_to_constant_1): Use build1 > to build the ADDR_EXPR which we don't really want to simplify. > * tree-ssa-dom.c (record_equivalences_from_stmt): Likewise. > * tree-ssa-loop-im.c (gather_mem_refs_stmt): Likewise. > * tree-ssa-forwprop.c (forward_propagate_addr_expr_1): Likewise. > (simplify_builtin_call): Strip useless type conversions. > * tree-ssa-strlen.c (new_strinfo): Likewise. > > * gcc.dg/pr94188.c: New testcase. Hi Richard, This breaks Linux kernel build on 32-bit ARM: 00:01:29 ./include/linux/string.h:333:9: internal compiler error: in gen_movsi, at config/arm/arm.md:6291 00:01:29 make[2]: *** [sound/drivers/serial-u16550.o] Error 1 Would you please investigate? Let me know if you need any help reproducing the problem. Kernel’s build line is (assuming cross-compilation): make CC=/path/to/arm-linux-gnueabihf-gcc ARCH=arm CROSS_COMPILE=arm-linux-gnueabihf- HOSTCC=gcc allyesconfig Regards, -- Maxim Kuvyrkov https://www.linaro.org
Re: One more patch for PR91333
> On Feb 18, 2020, at 6:30 PM, Vladimir Makarov wrote: > > On 2/17/20 10:08 AM, Maxim Kuvyrkov wrote: >> Hi Vladimir, >> >> This patch increases code size at -Os on arm-linux-gnueabihf by 1% (with no >> code-size reductions) on several SPEC CPU2006 benchmarks: >> >> 400.perlbench,perlbench_base.default ,580842,583842 >> 429.mcf,mcf_base.default ,7867,7955 >> 403.gcc,gcc_base.default ,1726449,1736149 >> 433.milc,milc_base.default ,66328,66816 >> 456.hmmer,hmmer_base.default ,148394,149434 >> 482.sphinx3,sphinx_livepretend_base.default ,99183,99863 >> >> Could you look into whether these regressions can be avoided? >> > Sure, Maxim. I'll look into it. If I can not solve the problem, I probably > revert the patch. Thanks, Vladimir. Forgot to mention that this is for Thumb2 (--target=arm-linux-gnueabihf --with-mode=thumb). Regards, -- Maxim Kuvyrkov https://www.linaro.org
Re: One more patch for PR91333
Hi Vladimir, This patch increases code size at -Os on arm-linux-gnueabihf by 1% (with no code-size reductions) on several SPEC CPU2006 benchmarks: 400.perlbench,perlbench_base.default ,580842,583842 429.mcf,mcf_base.default ,7867,7955 403.gcc,gcc_base.default ,1726449,1736149 433.milc,milc_base.default ,66328,66816 456.hmmer,hmmer_base.default ,148394,149434 482.sphinx3,sphinx_livepretend_base.default ,99183,99863 Could you look into whether these regressions can be avoided? Thanks, -- Maxim Kuvyrkov https://www.linaro.org > On Feb 2, 2020, at 7:46 PM, Vladimir Makarov wrote: > > The previous patch for > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91333 > > resulted in aarch64 testsuite failures. > > The following patch solves some of the failures and modified the PR test as > the generated code changed. > > The patch was successfully bootstrapped on x86-64 and benchmarked on SPEC2000. > > >
Re: [PATCH] simplify-rtx: Extend (truncate (*extract ...)) fold [PR87763]
Hi Richard, You patch below increases code-size on aarch64-linux-gnu with -Os on SPEC2k6 400.perlbench and 453.povray -- by 1% and 2% respectively. 400.perlbench,perlbench_base.default, 101,939261,951221 453.povray,povray_base.default, 102,707807,721399 Would you please check whether these can be avoided? [Let me know if you need help reproducing this problem.] Thank you, -- Maxim Kuvyrkov https://www.linaro.org > On Jan 27, 2020, at 7:41 PM, Richard Sandiford > wrote: > > In the gcc.target/aarch64/lsl_asr_sbfiz.c part of this PR, we have: > > Failed to match this instruction: > (set (reg:SI 95) >(ashift:SI (subreg:SI (sign_extract:DI (subreg:DI (reg:SI 97) 0) >(const_int 3 [0x3]) >(const_int 0 [0])) 0) >(const_int 19 [0x13]))) > > If we perform the natural simplification to: > > (set (reg:SI 95) >(ashift:SI (sign_extract:SI (reg:SI 97) >(const_int 3 [0x3]) >(const_int 0 [0])) 0) >(const_int 19 [0x13]))) > > then the pattern matches. And it turns out that we do have a > simplification like that already, but it would only kick in for > extractions from a reg, not a subreg. E.g.: > > (set (reg:SI 95) >(ashift:SI (subreg:SI (sign_extract:DI (reg:DI X) >(const_int 3 [0x3]) >(const_int 0 [0])) 0) >(const_int 19 [0x13]))) > > would simplify to: > > (set (reg:SI 95) >(ashift:SI (sign_extract:SI (subreg:SI (reg:DI X) 0) >(const_int 3 [0x3]) >(const_int 0 [0])) 0) >(const_int 19 [0x13]))) > > IMO the subreg case is even more obviously a simplification > than the bare reg case, since the net effect is to remove > either one or two subregs, rather than simply change the > position of a subreg/truncation. > > However, doing that regressed gcc.dg/tree-ssa/pr64910-2.c > for -m32 on x86_64-linux-gnu, because we could then simplify > a :HI zero_extract to a :QI one. The associated *testqi_ext_3 > pattern did already seem to want to handle QImode extractions: > > "ix86_match_ccmode (insn, CCNOmode) > && ((TARGET_64BIT && GET_MODE (operands[2]) == DImode) > || GET_MODE (operands[2]) == SImode > || GET_MODE (operands[2]) == HImode > || GET_MODE (operands[2]) == QImode) > > but I'm not sure how often the QI case would trigger in practice, > since the zero_extract mode was restricted to HI and above. I checked > the other x86 patterns and couldn't see any other instances of this. > > Tested on aarch64-linux-gnu, x86_64-linux-gnu and powerpc64le-linux-gnu, > OK to install? > > Richard > > > 2020-01-27 Richard Sandiford > > gcc/ > PR rtl-optimization/87763 > * simplify-rtx.c (simplify_truncation): Extend sign/zero_extract > simplification to handle subregs as well as bare regs. > * config/i386/i386.md (*testqi_ext_3): Match QI extracts too. > --- > gcc/config/i386/i386.md | 2 +- > gcc/simplify-rtx.c | 4 +++- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md > index 6e9c9bd2fb6..a125ab350bb 100644 > --- a/gcc/config/i386/i386.md > +++ b/gcc/config/i386/i386.md > @@ -8927,7 +8927,7 @@ (define_insn "*testqi_ext_2" > (define_insn_and_split "*testqi_ext_3" > [(set (match_operand 0 "flags_reg_operand") > (match_operator 1 "compare_operator" > - [(zero_extract:SWI248 > + [(zero_extract:SWI >(match_operand 2 "nonimmediate_operand" "rm") >(match_operand 3 "const_int_operand" "n") >(match_operand 4 "const_int_operand" "n")) > diff --git a/gcc/simplify-rtx.c b/gcc/simplify-rtx.c > index eff1d07a253..db4f9339c15 100644 > --- a/gcc/simplify-rtx.c > +++ b/gcc/simplify-rtx.c > @@ -736,7 +736,9 @@ simplify_truncation (machine_mode mode, rtx op, > (*_extract:M1 (truncate:M1 (reg:M2)) (len) (pos')) if possible without > changing len. */ > if ((GET_CODE (op) == ZERO_EXTRACT || GET_CODE (op) == SIGN_EXTRACT) > - && REG_P (XEXP (op, 0)) > + && (REG_P (XEXP (op, 0)) > + || (SUBREG_P (XEXP (op, 0)) > + && REG_P (SUBREG_REG (XEXP (op, 0) > && GET_MODE (XEXP (op, 0)) == GET_MODE (op) > && CONST_INT_P (XEXP (op, 1)) > && CONST_INT_P (XEXP (op, 2))) > -- > 2.17.1 >
Re: [Contrib PATCH] Add scripts to convert GCC repo from SVN to Git
> On Aug 24, 2019, at 12:30 AM, Joseph Myers wrote: > > On Fri, 23 Aug 2019, Maxim Kuvyrkov wrote: > >> I propose that we switch to gcc-pretty.git repository, because it has >> accurate Committer and Author fields. Developer names and email >> addresses are extracted from source history, and accurately track people >> changing companies, email addresses, and names. IMO, it is more >> important for people to get credit for open-source contributions on >> github, ohloh, etc., than the inconvenience of rebasing local git >> branches. It's also an important marketing tool for open-source >> companies to show stats of their corporate email addresses appearing in >> git commit logs. > > I concur that accurately crediting contributors is important and means we > should not start from the existing mirror (though we should keep its > branches available, so references to them and to their commit hashes > continue to work - either keeping the existing repository available under > a different name, or renaming the branches to put them in the new > repository - which should not enlarge the repository much because blob and > tree objects will generally be shared between the two versions of the > history). > > I note that the Go conversion of reposurgeon is now just five test > failures away from passing the whole reposurgeon testsuite (at which point > it should be ready for an attempt on the GCC conversion). Given the good > progress being made there at present, I thus suggest we plan to compare > this conversion with one from reposurgeon (paying special attention to the > messiest parts of the repository, such as artifacts from cvs2svn > attempting to locate branchpoints), unless those last five goreposurgeon > test failures prove unexpectedly time-consuming to get resolved. Could you upload GCC repo converted with reposurgeon somewhere public? And also list expected artifacts in its current version? >From my side, the machine on which the conversion ran ran out of disk space >about 3 weeks ago. I'll clean it up and restart the conversion updates. I'll also improve author entries a bit, so gcc-pretty.git's history will change ever so slightly. > > There are of course plenty of things to do relating to a git conversion > that do not depend on the particular choice of a converted repository - > such as writing git hooks and git versions of the maintainer-scripts > scripts that currently work with SVN, or working out a specific choice of > how to arrange annotated tags to allow "git describe" to give the sort of > monotonic version number some contributors want. > > A reasonable starting point for hooks would be that they closely > approximate what the current SVN hooks do for commit mails to gcc-cvs and > for Bugzilla updates, as what the current hooks do is clearly OK at > present and we shouldn't need to entangle substantive changes to what the > hooks do with the actual conversion to git; we can always discuss changes > later. Would the community please assign a volunteer for this at Cauldron? :-P Thank you, -- Maxim Kuvyrkov www.linaro.org
Re: [PR91598] Improve autoprefetcher heuristic in haifa-sched.c
> On Aug 29, 2019, at 7:29 PM, Richard Biener > wrote: > > On August 29, 2019 5:40:47 PM GMT+02:00, Maxim Kuvyrkov > wrote: >> Hi, >> >> This patch tweaks autoprefetcher heuristic in scheduler to better group >> memory loads and stores together. >> >> From https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91598: >> >> There are two separate changes, both related to instruction scheduler, >> that cause the regression. The first change in r253235 is responsible >> for 70% of the regression. >> === >> haifa-sched: fix autopref_rank_for_schedule qsort comparator >> >> * haifa-sched.c (autopref_rank_for_schedule): Order 'irrelevant' insns >> first, always call autopref_rank_data otherwise. >> >> >> >> git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@253235 >> 138bc75d-0d04-0410-961f-82ee72b054a4 >> === >> >> After this change instead of >> r1 = [rb + 0] >> r2 = [rb + 8] >> r3 = [rb + 16] >> r4 = >> r5 = >> r6 = >> >> we get >> r1 = [rb + 0] >> >> r2 = [rb + 8] >> >> r3 = [rb + 16] >> >> >> which, apparently, cortex-a53 autoprefetcher doesn't recognize. This >> schedule happens because r2= load gets lower priority than the >> "irrelevant" due to the above patch. >> >> If we think about it, the fact that "r1 = [rb + 0]" can be scheduled >> means that true dependencies of all similar base+offset loads are >> resolved. Therefore, for autoprefetcher-friendly schedule we should >> prioritize memory reads before "irrelevant" instructions. > > But isn't there also max number of load issues in a fetch window to consider? > So interleaving arithmetic with loads might be profitable. It appears that cores with autoprefetcher hardware prefer loads and stores bundled together, not interspersed with other instructions to occupy the rest of CPU units. Autoprefetching heuristic is enabled only for cores that support it, and isn't active for by default. > >> On the other hand, following similar logic, we want to delay memory >> stores as much as possible to start scheduling them only after all >> potential producers are scheduled. I.e., for autoprefetcher-friendly >> schedule we should prioritize "irrelevant" instructions before memory >> writes. >> >> Obvious patch to implement the above is attached. It brings 70% of >> regressed performance on this testcase back. >> >> OK to commit? >> >> Regards, >> >> -- >> Maxim Kuvyrkov >> www.linaro.org
[PATCH] Couple of debug dump improvements to scheduler (no code-gen changes)
Hi, The first patch adds ranking statistics for autoprefetcher heuristic. The second one makes it easier to diff scheduler debug dumps by adding more context lines for diff at clock increments. OK to commit? -- Maxim Kuvyrkov www.linaro.org 0002-Add-missing-entry-for-rank_for_schedule-stats.patch Description: Binary data 0003-Improve-diff-ability-of-scheduler-logs.patch Description: Binary data
[PR91598] Improve autoprefetcher heuristic in haifa-sched.c
Hi, This patch tweaks autoprefetcher heuristic in scheduler to better group memory loads and stores together. From https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91598: There are two separate changes, both related to instruction scheduler, that cause the regression. The first change in r253235 is responsible for 70% of the regression. === haifa-sched: fix autopref_rank_for_schedule qsort comparator * haifa-sched.c (autopref_rank_for_schedule): Order 'irrelevant' insns first, always call autopref_rank_data otherwise. git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@253235 138bc75d-0d04-0410-961f-82ee72b054a4 === After this change instead of r1 = [rb + 0] r2 = [rb + 8] r3 = [rb + 16] r4 = r5 = r6 = we get r1 = [rb + 0] r2 = [rb + 8] r3 = [rb + 16] which, apparently, cortex-a53 autoprefetcher doesn't recognize. This schedule happens because r2= load gets lower priority than the "irrelevant" due to the above patch. If we think about it, the fact that "r1 = [rb + 0]" can be scheduled means that true dependencies of all similar base+offset loads are resolved. Therefore, for autoprefetcher-friendly schedule we should prioritize memory reads before "irrelevant" instructions. On the other hand, following similar logic, we want to delay memory stores as much as possible to start scheduling them only after all potential producers are scheduled. I.e., for autoprefetcher-friendly schedule we should prioritize "irrelevant" instructions before memory writes. Obvious patch to implement the above is attached. It brings 70% of regressed performance on this testcase back. OK to commit? Regards, -- Maxim Kuvyrkov www.linaro.org 0001-Improve-autoprefetcher-heuristic-partly-fix-regressi.patch Description: Binary data
Re: [Contrib PATCH] Add scripts to convert GCC repo from SVN to Git
> On Aug 6, 2019, at 12:32 PM, Maxim Kuvyrkov wrote: > ... > I've setup uploads and updates of fully converted GCC history (all branches > and all tags) in 3 flavors. These will be updated roughly hourly. > > 1. https://git-us.linaro.org/people/maxim-kuvyrkov/gcc-pretty.git/ > This is a fresh conversion from scratch with "pretty" authors. > > 2. https://git.linaro.org/people/maxim-kuvyrkov/gcc-mirror.git/ > This is a close match to current GCC mirror. Trunk and gcc-*-branch branches > are imported from the mirror, and the rest is reconstructed starting from the > imported branches. > > 3. https://git-us.linaro.org/people/maxim-kuvyrkov/gcc-raw.git/ > This is a fresh conversion from scratch with no author rewrites. > The conversion is now fully complete. The above 3 repositories all have complete and accurate [1] history of all branches and tags. SVN's /branches/* are converted to Git's refs/heads/*, and SVN's /tags/* are converted to Git's annotated tags refs/tags/*. SVN's /trunk is Git's refs/heads/master. I propose that we switch to gcc-pretty.git repository, because it has accurate Committer and Author fields. Developer names and email addresses are extracted from source history, and accurately track people changing companies, email addresses, and names. IMO, it is more important for people to get credit for open-source contributions on github, ohloh, etc., than the inconvenience of rebasing local git branches. It's also an important marketing tool for open-source companies to show stats of their corporate email addresses appearing in git commit logs. I also suggest that we don't wait for Cauldron to make plan on when and how to switch. I believe the big decisions should be made on the mailing list, and at Cauldron we can discuss finer points. [Also, unfortunately, I won't attend this year.] [1] Gcc-mirror.git has artifacts in several commit messages due to edits of SVN commit messages after the fact. Regards, -- Maxim Kuvyrkov www.linaro.org
Re: [Contrib PATCH] Add scripts to convert GCC repo from SVN to Git
> On Aug 5, 2019, at 11:24 AM, Maxim Kuvyrkov wrote: > > >> On Aug 2, 2019, at 11:41 AM, Maxim Kuvyrkov >> wrote: >> >>> On Aug 1, 2019, at 11:43 PM, Jason Merrill wrote: >>> > ... >>>> Unfortunately, current mirror does not and could not account for rewrites >>>> of SVN commit log messages. For trunk the histories of diverge in 2008 >>>> due to commit message change of r138154. This is not a single occurrence; >>>> I've compared histories only of trunk and gcc-6-branch, and both had >>>> commit message change (for gcc-6-branch see r259978). >>>> >>>> It's up to the community is to weigh pros and cons of re-using existing >>>> GCC mirror as conversion base vs regenerating history from scratch: >>>> >>>> Pros of using GCC mirror: >>>> + No need to rebase public git-only branches >>>> + No need to rebase private branches >>>> + No need to rebase current clones, checkouts, work-in-progress trees >>>> >>>> Cons of using GCC mirror: >>>> - Poor author / committer IDs (this breaks patch statistics software) >>>> - Several commit messages will not be the current "fixed" version >>>> >>>> Thoughts? >>> >>> I'm still inclined to stick with the mirror. I would expect patch >>> statistics software to be able to be taught about multiple addresses >>> for the same person. >> >> Patch tracking software breaks on emails like >> , where >> 38bc75d-0d04-0410-961f-82ee72b054a4 is not a reasonable domain name. >> >> For completeness, I'll generate and upload a repo based on current mirror >> with all branches and tags converted. > > Yeah, this didn't worked as well as I hoped. Current gcc git mirror has > wrong history for branches that followed scenario: > 1. create $branch from $base at revision N > 2. commit WORK on $branch > 3. delete $branch > 4. create $branch from $base at revision N+M > 5. rebase WORK on current $branch > > Current mirror connects histories of two versions of $branch, and we get > wrong history. In step (4) instead of plain history of $base we get a commit > merging histories of $branch just before deletion and $base at revision N+M. > > There are many branches like this, e.g., branches/gccgo. I've setup uploads and updates of fully converted GCC history (all branches and all tags) in 3 flavors. These will be updated roughly hourly. 1. https://git-us.linaro.org/people/maxim-kuvyrkov/gcc-pretty.git/ This is a fresh conversion from scratch with "pretty" authors. 2. https://git.linaro.org/people/maxim-kuvyrkov/gcc-mirror.git/ This is a close match to current GCC mirror. Trunk and gcc-*-branch branches are imported from the mirror, and the rest is reconstructed starting from the imported branches. 3. https://git-us.linaro.org/people/maxim-kuvyrkov/gcc-raw.git/ This is a fresh conversion from scratch with no author rewrites. -- Maxim Kuvyrkov www.linaro.org
Re: [Contrib PATCH] Add scripts to convert GCC repo from SVN to Git
> On Aug 2, 2019, at 11:41 AM, Maxim Kuvyrkov wrote: > >> On Aug 1, 2019, at 11:43 PM, Jason Merrill wrote: >> ... >>> Unfortunately, current mirror does not and could not account for rewrites >>> of SVN commit log messages. For trunk the histories of diverge in 2008 due >>> to commit message change of r138154. This is not a single occurrence; I've >>> compared histories only of trunk and gcc-6-branch, and both had commit >>> message change (for gcc-6-branch see r259978). >>> >>> It's up to the community is to weigh pros and cons of re-using existing GCC >>> mirror as conversion base vs regenerating history from scratch: >>> >>> Pros of using GCC mirror: >>> + No need to rebase public git-only branches >>> + No need to rebase private branches >>> + No need to rebase current clones, checkouts, work-in-progress trees >>> >>> Cons of using GCC mirror: >>> - Poor author / committer IDs (this breaks patch statistics software) >>> - Several commit messages will not be the current "fixed" version >>> >>> Thoughts? >> >> I'm still inclined to stick with the mirror. I would expect patch >> statistics software to be able to be taught about multiple addresses >> for the same person. > > Patch tracking software breaks on emails like > , where > 38bc75d-0d04-0410-961f-82ee72b054a4 is not a reasonable domain name. > > For completeness, I'll generate and upload a repo based on current mirror > with all branches and tags converted. Yeah, this didn't worked as well as I hoped. Current gcc git mirror has wrong history for branches that followed scenario: 1. create $branch from $base at revision N 2. commit WORK on $branch 3. delete $branch 4. create $branch from $base at revision N+M 5. rebase WORK on current $branch Current mirror connects histories of two versions of $branch, and we get wrong history. In step (4) instead of plain history of $base we get a commit merging histories of $branch just before deletion and $base at revision N+M. There are many branches like this, e.g., branches/gccgo. -- Maxim Kuvyrkov www.linaro.org
Re: [Contrib PATCH] Add scripts to convert GCC repo from SVN to Git
> On Aug 2, 2019, at 2:06 PM, Richard Biener wrote: > > On Fri, Aug 2, 2019 at 1:01 PM Martin Liška wrote: >> >> On 8/2/19 12:54 PM, Maxim Kuvyrkov wrote: >>>> On Aug 2, 2019, at 1:26 PM, Martin Liška wrote: >>>> >>>> On 8/2/19 10:41 AM, Maxim Kuvyrkov wrote: >>>>> In the end, I don't care much to which version of the repo we switch, as >>>>> long as we switch. >>>> >>>> Hi Maxim. >>>> >>>> I really appreciate that you've been working on that. Same as you I would >>>> like to see >>>> any change that will lead to a git repository. >>>> >>>> I have couple of questions about the upcoming Cauldron: >>>> >>>> - Are you planning to attend? >>> >>> Unfortunately, I won't attend this time. >> >> I see. >> >>> >>>> - Would it be possible to prepare a voting during e.g. Steering Committee >>>> where >>>> we'll vote about transition options? >>>> - Would it make sense to do an online questionnaire in advance in order >>>> to guess what's prevailing opinion? >>>> >>>> If you are interested, I can help you? >>> >>> Let's organize an online survey now. While most active GCC developers will >>> attend Cauldron, many others will not, so we shouldn't rely on Cauldron to >>> make any final decisions. >> >> Sure, online is the best option as all active community members can vote. >> >>> >>> Martin, would you please organize the survey? >> >> Yes, but I haven't followed the discussion in recent weeks. Is the only >> question >> whether we want the current GIT mirror or your rebased git repository? >> Is Eric Raymond's transition still in play or not? > > 1) Stay with SVN > 2) Switch to the existing GIT mirror > 3) Wait for ERS to complete his conversion to GIT > 4) Use the existing new conversion to GIT fixing authors and commit messages > 5) I don't care > 6) I don't care as long as we switch to GIT > >> Are there any other sub-question regarding commit message format, git hooks, >> etc. >> that will deserve a place in the questionnaire? > > No, please do not make it unnecessarily complicated. Maybe the questionaire > can include a free-form text field for more comments. > > Btw, I do not believe we should do this kind of voting. Who's eligible to > vote? > Is the vote anonymous? What happens when the majority (what is the majority?) > votes for option N? > > IMHO voting is bike-shedding. > > Those who do the work decide. _They_ may ask questions _and_ decide whether > to listen to the answer. I think we should do a /survey/, not organize a vote. From reading this thread an independent observer will get an impression that GCC developers are split into 3 roughly equal parts: 1. those who prefer switch to existing mirror, 2. those who prefer to wait for reposurgeon conversion, 3. those who prefer to switch to svn-git conversion with authors fixed. The survey might show that we have a clear majority for one of the options, and that we can conclude the discussion. If survey shows that we don't have a clear winner, then let's continue the discussion. IMO, anyone who considers himself or herself a GCC developer should participate and survey should not be anonymous to avoid abuse. -- Maxim Kuvyrkov www.linaro.org
Re: [Contrib PATCH] Add scripts to convert GCC repo from SVN to Git
> On Aug 2, 2019, at 11:35 AM, Maxim Kuvyrkov wrote: > >> On Jul 22, 2019, at 12:05 PM, Maxim Kuvyrkov >> wrote: >> > ... >>>>> As far as GCC conversion goes, below is what I plan to do and what not to >>>>> do. This is based on comments from everyone in this thread: >>>>> >>>>> 1. Construct GCC's git repo from SVN using same settings as current git >>>>> mirror. >>>>> 2. Compare the resulting git repo with current GCC mirror -- they should >>>>> match on the commit hash level for trunk, branches/gcc-*-branch, and >>>>> other "normal" branches. >>>>> 3. Investigate any differences between converted GCC repo and current GCC >>>>> mirror. These can be due to bugs in git-svn or other misconfigurations. >>>>> 4. Import git-only branches from current GCC mirror. >>>>> 5. Publish this "raw" repo for community to sanity-check its contents. >>>> >>>> Why not start from the current mirror? Perhaps a mirror of the mirror? >>> >>> To check that git-svn is self-consistent and generates same commits now as >>> it was several years ago when you setup the current mirror. >> >> Unfortunately, current mirror does not and could not account for rewrites of >> SVN commit log messages. For trunk the histories of diverge in 2008 due to >> commit message change of r138154. This is not a single occurrence; I've >> compared histories only of trunk and gcc-6-branch, and both had commit >> message change (for gcc-6-branch see r259978). >> >> It's up to the community is to weigh pros and cons of re-using existing GCC >> mirror as conversion base vs regenerating history from scratch: >> >> Pros of using GCC mirror: >> + No need to rebase public git-only branches >> + No need to rebase private branches >> + No need to rebase current clones, checkouts, work-in-progress trees >> >> Cons of using GCC mirror: >> - Poor author / committer IDs (this breaks patch statistics software) >> - Several commit messages will not be the current "fixed" version >> >> Thoughts? > > Ping? > > Meanwhile, status update: > > 1. GitHub blocked https://github.com/maxim-kuvyrkov/gcc/ due to excessive > resource usage. I've asked them to unblock and explained why pushes are as > big as they are. > > 2. I'm uploading to https://git.linaro.org/people/maxim.kuvyrkov/gcc.git/ for > now. The correct link is https://git.linaro.org/people/maxim-kuvyrkov/gcc.git/ . Thanks to Segher for pointing this out. > > 3. Conversion is now feature-complete. The scripts re-write author and > committer fields, as well as create proper git tags. > > 4. "Raw" version of repository is available under refs/raw/*. > > 5. refs/raw/* are not expected to change. > > 6. refs/heads/* and refs/tags/* might change due to author/committer name > fixes and improvements. > > Please scrutinize the repo and let me know of any artifacts. -- Maxim Kuvyrkov www.linaro.org
Re: [Contrib PATCH] Add scripts to convert GCC repo from SVN to Git
> On Aug 2, 2019, at 1:26 PM, Martin Liška wrote: > > On 8/2/19 10:41 AM, Maxim Kuvyrkov wrote: >> In the end, I don't care much to which version of the repo we switch, as >> long as we switch. > > Hi Maxim. > > I really appreciate that you've been working on that. Same as you I would > like to see > any change that will lead to a git repository. > > I have couple of questions about the upcoming Cauldron: > > - Are you planning to attend? Unfortunately, I won't attend this time. > - Would it be possible to prepare a voting during e.g. Steering Committee > where > we'll vote about transition options? > - Would it make sense to do an online questionnaire in advance in order > to guess what's prevailing opinion? > > If you are interested, I can help you? Let's organize an online survey now. While most active GCC developers will attend Cauldron, many others will not, so we shouldn't rely on Cauldron to make any final decisions. Martin, would you please organize the survey? Regards, -- Maxim Kuvyrkov www.linaro.org
Re: [Contrib PATCH] Add scripts to convert GCC repo from SVN to Git
> On Aug 1, 2019, at 11:43 PM, Jason Merrill wrote: > > On Mon, Jul 22, 2019 at 5:05 AM Maxim Kuvyrkov > wrote: >> >>> On Jul 16, 2019, at 5:14 PM, Maxim Kuvyrkov >>> wrote: >>> >>>> On Jul 16, 2019, at 3:34 PM, Jason Merrill wrote: >>>> >>>> On Tue, Jul 16, 2019 at 12:18 PM Maxim Kuvyrkov >>>> wrote: >>>>> >>>>> Hi Everyone, >>>>> >>>>> I've been swamped with other projects for most of June, which gave me >>>>> time to digest all the feedback I've got on GCC's conversion from SVN to >>>>> Git. >>>>> >>>>> The scripts have heavily evolved from the initial version posted here. >>>>> They have become fairly generic in that they have no implied knowledge >>>>> about GCC's repo structure. Due to this I no longer plan to merge them >>>>> into GCC tree, but rather publish as a separate project on github. For >>>>> now, you can track the current [hairy] version at >>>>> https://review.linaro.org/c/toolchain/gcc/+/31416 . >>>>> >>>>> The initial version of scripts used heuristics to construct branch tree, >>>>> which turned out to be error-prone. The current version parse entire >>>>> history of SVN repo to detect all trees that start at /trunk@1. >>>>> Therefore all branches in the converted repo converge to the same parent >>>>> at the beginning of their histories. >>>>> >>>>> As far as GCC conversion goes, below is what I plan to do and what not to >>>>> do. This is based on comments from everyone in this thread: >>>>> >>>>> 1. Construct GCC's git repo from SVN using same settings as current git >>>>> mirror. >>>>> 2. Compare the resulting git repo with current GCC mirror -- they should >>>>> match on the commit hash level for trunk, branches/gcc-*-branch, and >>>>> other "normal" branches. >>>>> 3. Investigate any differences between converted GCC repo and current GCC >>>>> mirror. These can be due to bugs in git-svn or other misconfigurations. >>>>> 4. Import git-only branches from current GCC mirror. >>>>> 5. Publish this "raw" repo for community to sanity-check its contents. >>>> >>>> Why not start from the current mirror? Perhaps a mirror of the mirror? >>> >>> To check that git-svn is self-consistent and generates same commits now as >>> it was several years ago when you setup the current mirror. >> >> Unfortunately, current mirror does not and could not account for rewrites of >> SVN commit log messages. For trunk the histories of diverge in 2008 due to >> commit message change of r138154. This is not a single occurrence; I've >> compared histories only of trunk and gcc-6-branch, and both had commit >> message change (for gcc-6-branch see r259978). >> >> It's up to the community is to weigh pros and cons of re-using existing GCC >> mirror as conversion base vs regenerating history from scratch: >> >> Pros of using GCC mirror: >> + No need to rebase public git-only branches >> + No need to rebase private branches >> + No need to rebase current clones, checkouts, work-in-progress trees >> >> Cons of using GCC mirror: >> - Poor author / committer IDs (this breaks patch statistics software) >> - Several commit messages will not be the current "fixed" version >> >> Thoughts? > > I'm still inclined to stick with the mirror. I would expect patch > statistics software to be able to be taught about multiple addresses > for the same person. Patch tracking software breaks on emails like , where 38bc75d-0d04-0410-961f-82ee72b054a4 is not a reasonable domain name. For completeness, I'll generate and upload a repo based on current mirror with all branches and tags converted. In the end, I don't care much to which version of the repo we switch, as long as we switch. -- Maxim Kuvyrkov www.linaro.org
Re: [Contrib PATCH] Add scripts to convert GCC repo from SVN to Git
> On Jul 22, 2019, at 12:05 PM, Maxim Kuvyrkov > wrote: > ... >>>> As far as GCC conversion goes, below is what I plan to do and what not to >>>> do. This is based on comments from everyone in this thread: >>>> >>>> 1. Construct GCC's git repo from SVN using same settings as current git >>>> mirror. >>>> 2. Compare the resulting git repo with current GCC mirror -- they should >>>> match on the commit hash level for trunk, branches/gcc-*-branch, and other >>>> "normal" branches. >>>> 3. Investigate any differences between converted GCC repo and current GCC >>>> mirror. These can be due to bugs in git-svn or other misconfigurations. >>>> 4. Import git-only branches from current GCC mirror. >>>> 5. Publish this "raw" repo for community to sanity-check its contents. >>> >>> Why not start from the current mirror? Perhaps a mirror of the mirror? >> >> To check that git-svn is self-consistent and generates same commits now as >> it was several years ago when you setup the current mirror. > > Unfortunately, current mirror does not and could not account for rewrites of > SVN commit log messages. For trunk the histories of diverge in 2008 due to > commit message change of r138154. This is not a single occurrence; I've > compared histories only of trunk and gcc-6-branch, and both had commit > message change (for gcc-6-branch see r259978). > > It's up to the community is to weigh pros and cons of re-using existing GCC > mirror as conversion base vs regenerating history from scratch: > > Pros of using GCC mirror: > + No need to rebase public git-only branches > + No need to rebase private branches > + No need to rebase current clones, checkouts, work-in-progress trees > > Cons of using GCC mirror: > - Poor author / committer IDs (this breaks patch statistics software) > - Several commit messages will not be the current "fixed" version > > Thoughts? Ping? Meanwhile, status update: 1. GitHub blocked https://github.com/maxim-kuvyrkov/gcc/ due to excessive resource usage. I've asked them to unblock and explained why pushes are as big as they are. 2. I'm uploading to https://git.linaro.org/people/maxim.kuvyrkov/gcc.git/ for now. 3. Conversion is now feature-complete. The scripts re-write author and committer fields, as well as create proper git tags. 4. "Raw" version of repository is available under refs/raw/*. 5. refs/raw/* are not expected to change. 6. refs/heads/* and refs/tags/* might change due to author/committer name fixes and improvements. Please scrutinize the repo and let me know of any artifacts. -- Maxim Kuvyrkov www.linaro.org
Re: [Contrib PATCH] Add scripts to convert GCC repo from SVN to Git
> On Jul 16, 2019, at 5:14 PM, Maxim Kuvyrkov wrote: > >> On Jul 16, 2019, at 3:34 PM, Jason Merrill wrote: >> >> On Tue, Jul 16, 2019 at 12:18 PM Maxim Kuvyrkov >> wrote: >>> >>> Hi Everyone, >>> >>> I've been swamped with other projects for most of June, which gave me time >>> to digest all the feedback I've got on GCC's conversion from SVN to Git. >>> >>> The scripts have heavily evolved from the initial version posted here. >>> They have become fairly generic in that they have no implied knowledge >>> about GCC's repo structure. Due to this I no longer plan to merge them >>> into GCC tree, but rather publish as a separate project on github. For >>> now, you can track the current [hairy] version at >>> https://review.linaro.org/c/toolchain/gcc/+/31416 . >>> >>> The initial version of scripts used heuristics to construct branch tree, >>> which turned out to be error-prone. The current version parse entire >>> history of SVN repo to detect all trees that start at /trunk@1. Therefore >>> all branches in the converted repo converge to the same parent at the >>> beginning of their histories. >>> >>> As far as GCC conversion goes, below is what I plan to do and what not to >>> do. This is based on comments from everyone in this thread: >>> >>> 1. Construct GCC's git repo from SVN using same settings as current git >>> mirror. >>> 2. Compare the resulting git repo with current GCC mirror -- they should >>> match on the commit hash level for trunk, branches/gcc-*-branch, and other >>> "normal" branches. >>> 3. Investigate any differences between converted GCC repo and current GCC >>> mirror. These can be due to bugs in git-svn or other misconfigurations. >>> 4. Import git-only branches from current GCC mirror. >>> 5. Publish this "raw" repo for community to sanity-check its contents. >> >> Why not start from the current mirror? Perhaps a mirror of the mirror? > > To check that git-svn is self-consistent and generates same commits now as it > was several years ago when you setup the current mirror. Unfortunately, current mirror does not and could not account for rewrites of SVN commit log messages. For trunk the histories of diverge in 2008 due to commit message change of r138154. This is not a single occurrence; I've compared histories only of trunk and gcc-6-branch, and both had commit message change (for gcc-6-branch see r259978). It's up to the community is to weigh pros and cons of re-using existing GCC mirror as conversion base vs regenerating history from scratch: Pros of using GCC mirror: + No need to rebase public git-only branches + No need to rebase private branches + No need to rebase current clones, checkouts, work-in-progress trees Cons of using GCC mirror: - Poor author / committer IDs (this breaks patch statistics software) - Several commit messages will not be the current "fixed" version Thoughts? -- Maxim Kuvyrkov www.linaro.org
Re: [Contrib PATCH] Add scripts to convert GCC repo from SVN to Git
> On Jul 16, 2019, at 5:14 PM, Maxim Kuvyrkov wrote: > >> On Jul 16, 2019, at 3:34 PM, Jason Merrill wrote: >> ... >> >>> b. Re-write tags/ branches into annotated tags. Note that tags/* are >>> included into history of several branches via merge or copy commits, so we >>> would need to re-write history to have proper references to annotated tag >>> commits in the histories of such branches. >> >> Missing tags is definitely something to fix about the current mirror. >> I don't think we need to worry about inserting them into branch >> history. > > If we don't do this then "git branch -a --contains some/tag" will not work > correctly. I was wrong here. Git tag objects (annotated tags) cannot appear in branch history because they are resolved to the commits they are pointing to. Only commit objects can appear in branch history. This makes conversion of tags much simpler, since [annotated] tags cannot affect history branches. Regards, -- Maxim Kuvyrkov www.linaro.org
Re: [Contrib PATCH] Add scripts to convert GCC repo from SVN to Git
> On Jul 16, 2019, at 3:34 PM, Jason Merrill wrote: > > On Tue, Jul 16, 2019 at 12:18 PM Maxim Kuvyrkov > wrote: >> >> Hi Everyone, >> >> I've been swamped with other projects for most of June, which gave me time >> to digest all the feedback I've got on GCC's conversion from SVN to Git. >> >> The scripts have heavily evolved from the initial version posted here. They >> have become fairly generic in that they have no implied knowledge about >> GCC's repo structure. Due to this I no longer plan to merge them into GCC >> tree, but rather publish as a separate project on github. For now, you can >> track the current [hairy] version at >> https://review.linaro.org/c/toolchain/gcc/+/31416 . >> >> The initial version of scripts used heuristics to construct branch tree, >> which turned out to be error-prone. The current version parse entire >> history of SVN repo to detect all trees that start at /trunk@1. Therefore >> all branches in the converted repo converge to the same parent at the >> beginning of their histories. >> >> As far as GCC conversion goes, below is what I plan to do and what not to >> do. This is based on comments from everyone in this thread: >> >> 1. Construct GCC's git repo from SVN using same settings as current git >> mirror. >> 2. Compare the resulting git repo with current GCC mirror -- they should >> match on the commit hash level for trunk, branches/gcc-*-branch, and other >> "normal" branches. >> 3. Investigate any differences between converted GCC repo and current GCC >> mirror. These can be due to bugs in git-svn or other misconfigurations. >> 4. Import git-only branches from current GCC mirror. >> 5. Publish this "raw" repo for community to sanity-check its contents. > > Why not start from the current mirror? Perhaps a mirror of the mirror? To check that git-svn is self-consistent and generates same commits now as it was several years ago when you setup the current mirror. > >> 6. Re-write history of all branches -- converted from svn and git-only -- >> see note below [*]. >> 7. Publish this "pretty" repo for community to sanity-check its contents. >> 8. Update both "raw" and "pretty" repos daily with new commits >> 9. Fix problems in the "raw" and "pretty" repos as they reported by the >> community. >> >> Once these steps are done, the community could switch from SVN to git by >> disabling commits to SVN, waiting for final history to be absorbed by the >> "pretty" repo, and deploying the git repo as the official repo. >> >> [*] Note on branch re-writing: >> During svn->git conversion we have an opportunity to correct some of the >> artifacts of current git mirror: >> >> a. Author and committer entries. These are difficult to get right during >> git-svn import process because the tool gives only SVN committer ID without >> much else. We could do much better by matching SVN committer ID with >> person's name in the map file, and then searching for person's >> current-at-the-time email address in the commit diff. I.e., mkuvyrkov -> >> Maxim Kuvyrkov -> [changelog from 2010's commit] -> ma...@codesourcery.com . > >> c. Since we are re-writing history anyway, it would be nice to convert >> "svn-git: svn+ssh://" tags to "svn-git: https://;. We are sure to retain >> publicly-visible svn repo accessible via https://, but not as likely to >> retain svn+ssh:// interface. > > I am moderately opposed to rewriting trunk and release branch history; > if we're using git-svn anyway, the benefit would have to be large to > outweigh the significant inconvenience to all current users of needing > to switch their local trees over to a new history. I mostly agree with your point. My thinking is that the git mirror was never official canonical GCC repo, and if we ever want to get better author/committer identities -- this is our chance. > >> b. Re-write tags/ branches into annotated tags. Note that tags/* are >> included into history of several branches via merge or copy commits, so we >> would need to re-write history to have proper references to annotated tag >> commits in the histories of such branches. > > Missing tags is definitely something to fix about the current mirror. > I don't think we need to worry about inserting them into branch > history. If we don't do this then "git branch -a --contains some/tag" will not work correctly. > > We should definitely also rewrite vendor/subdirectory b
Re: [Contrib PATCH] Add scripts to convert GCC repo from SVN to Git
Hi Everyone, I've been swamped with other projects for most of June, which gave me time to digest all the feedback I've got on GCC's conversion from SVN to Git. The scripts have heavily evolved from the initial version posted here. They have become fairly generic in that they have no implied knowledge about GCC's repo structure. Due to this I no longer plan to merge them into GCC tree, but rather publish as a separate project on github. For now, you can track the current [hairy] version at https://review.linaro.org/c/toolchain/gcc/+/31416 . The initial version of scripts used heuristics to construct branch tree, which turned out to be error-prone. The current version parse entire history of SVN repo to detect all trees that start at /trunk@1. Therefore all branches in the converted repo converge to the same parent at the beginning of their histories. As far as GCC conversion goes, below is what I plan to do and what not to do. This is based on comments from everyone in this thread: 1. Construct GCC's git repo from SVN using same settings as current git mirror. 2. Compare the resulting git repo with current GCC mirror -- they should match on the commit hash level for trunk, branches/gcc-*-branch, and other "normal" branches. 3. Investigate any differences between converted GCC repo and current GCC mirror. These can be due to bugs in git-svn or other misconfigurations. 4. Import git-only branches from current GCC mirror. 5. Publish this "raw" repo for community to sanity-check its contents. 6. Re-write history of all branches -- converted from svn and git-only -- see note below [*]. 7. Publish this "pretty" repo for community to sanity-check its contents. 8. Update both "raw" and "pretty" repos daily with new commits 9. Fix problems in the "raw" and "pretty" repos as they reported by the community. Once these steps are done, the community could switch from SVN to git by disabling commits to SVN, waiting for final history to be absorbed by the "pretty" repo, and deploying the git repo as the official repo. [*] Note on branch re-writing: During svn->git conversion we have an opportunity to correct some of the artifacts of current git mirror: a. Author and committer entries. These are difficult to get right during git-svn import process because the tool gives only SVN committer ID without much else. We could do much better by matching SVN committer ID with person's name in the map file, and then searching for person's current-at-the-time email address in the commit diff. I.e., mkuvyrkov -> Maxim Kuvyrkov -> [changelog from 2010's commit] -> ma...@codesourcery.com . b. Re-write tags/ branches into annotated tags. Note that tags/* are included into history of several branches via merge or copy commits, so we would need to re-write history to have proper references to annotated tag commits in the histories of such branches. c. Since we are re-writing history anyway, it would be nice to convert "svn-git: svn+ssh://" tags to "svn-git: https://;. We are sure to retain publicly-visible svn repo accessible via https://, but not as likely to retain svn+ssh:// interface. Which of these will make into the final repo is for community to decide. Regards, -- Maxim Kuvyrkov www.linaro.org > On May 28, 2019, at 1:31 PM, Maxim Kuvyrkov wrote: > > Hi Everyone, > > What can I say, I was too optimistic about how easy it would be to convert > GCC's svn repo to git one branch at a time. After 2 more weeks and several > re-writes of the scripts I now know more about GCC's svn history than I would > ever wanted. > > The prize for most complicated branch history goes to /branches/ibm/* . It > has merges, it has re-creation branches from /trunk and even an accidental > deletion of all of IBM's branches. > > The version of scripts I'm testing right now seems to deal with all of that. > > Also, to avoid controversy -- I'm working on these scripts to satisfy my own > curiosity, and to give GCC community another option to choose from for the > final migration. If by end of Summer 2019 we have 2-3 git repos to choose > from, then we are likely to push GCC [kicking and screaming] into 2010's by > the end of this decade. > > -- > Maxim Kuvyrkov > www.linaro.org > > > >> On May 14, 2019, at 7:11 PM, Maxim Kuvyrkov >> wrote: >> >> This patch adds scripts to contrib/ to migrate full history of GCC's >> subversion repository to git. My hope is that these scripts will finally >> allow GCC project to migrate to Git. >> >> The result of the conversion is at >> https://github.com/maxim-kuvyrkov/gcc/branches/all . Branches with "@rev" >> suffixes represent branch points. The conversion is still running, so n
Re: [Contrib PATCH] Add scripts to convert GCC repo from SVN to Git
Hi Everyone, What can I say, I was too optimistic about how easy it would be to convert GCC's svn repo to git one branch at a time. After 2 more weeks and several re-writes of the scripts I now know more about GCC's svn history than I would ever wanted. The prize for most complicated branch history goes to /branches/ibm/* . It has merges, it has re-creation branches from /trunk and even an accidental deletion of all of IBM's branches. The version of scripts I'm testing right now seems to deal with all of that. Also, to avoid controversy -- I'm working on these scripts to satisfy my own curiosity, and to give GCC community another option to choose from for the final migration. If by end of Summer 2019 we have 2-3 git repos to choose from, then we are likely to push GCC [kicking and screaming] into 2010's by the end of this decade. -- Maxim Kuvyrkov www.linaro.org > On May 14, 2019, at 7:11 PM, Maxim Kuvyrkov wrote: > > This patch adds scripts to contrib/ to migrate full history of GCC's > subversion repository to git. My hope is that these scripts will finally > allow GCC project to migrate to Git. > > The result of the conversion is at > https://github.com/maxim-kuvyrkov/gcc/branches/all . Branches with "@rev" > suffixes represent branch points. The conversion is still running, so not > all branches may appear right away. > > The scripts are not specific to GCC repo and are usable for other projects. > In particular, they should be able to convert downstream GCC svn repos. > > The scripts convert svn history branch by branch. They rely on git-svn on > convert individual branches. Git-svn is a good tool for converting > individual branches. It is, however, either very slow at converting the > entire GCC repo, or goes into infinite loop. > > There are 3 scripts: > > - svn-git-repo.sh: top level script to convert entire repo or a part of it > (e.g., branches/), > - svn-list-branches.sh: helper script to output branches and their parents in > bottom-up order, > - svn-git-branch.sh: helper script to convert a single branch. > > Whenever possible, svn-git-branch.sh uses existing git branches as caches. > > What are your questions and comments? > > The attached is cleaned up version, which hasn't been fully tested yet; typos > and other silly mistakes are likely. OK to commit after testing? > > -- > Maxim Kuvyrkov > www.linaro.org > > > <0001-Contrib-SVN-Git-conversion-scripts.patch>
Re: [Contrib PATCH] Add scripts to convert GCC repo from SVN to Git
> On May 17, 2019, at 4:07 PM, Jason Merrill wrote: > > On Tue, May 14, 2019 at 12:11 PM Maxim Kuvyrkov > wrote: >> >> This patch adds scripts to contrib/ to migrate full history of GCC's >> subversion repository to git. My hope is that these scripts will finally >> allow GCC project to migrate to Git. > > Thanks for looking into this. My feeling has been that, if we give up > on reposurgeon, there's no need to start a new conversion at all: we > can just switch the current mirror over to being the primary > repository with some minor surgery (e.g. using git filter-branch to > fix subdirectory branches). That approach will produce the least > disruption in the workflows of people already using it. Do you see a > problem with this? No technical problem. The scripts start with the existing git mirror and only convert the parts that are not present in it. FWIW, the scripts can start with a bare repo, but that would take longer time. It is a good idea to run a test conversion without using existing mirror as cache to confirm that there are no discrepancies in repos produced by old and new versions of git-svn. However, if the community decides that we want use author maps, then, iiuc, the new repo would not be compatible with the existing mirror. -- Maxim Kuvyrkov www.linaro.org
Re: [Contrib PATCH] Add scripts to convert GCC repo from SVN to Git
> On May 17, 2019, at 3:22 PM, Martin Liška wrote: > > On 5/17/19 1:06 AM, Joseph Myers wrote: >> That repository >> represents what I consider the collaboratively built consensus on such >> things as the desired author map (including handling of the ambiguous >> author name), which directories represent branches and tags, and what tags >> should be kept or removed - but building up such a consensus and keeping > > About the map. I agree with Richard that we should do best approach and not > to fully reconstruct history of people who has switched email address multi > times. I cloned git://thyrsus.com/repositories/gcc-conversion.git and made > a clean up: > > - for logins with duplicite emails I chose the latest one used on gcc-patches > mailing list > - comments were removed > - a few entries contained timezone and I stripped that > > Final version of the map can be seen here: > https://github.com/marxin/gcc-git-conversion/blob/cleanup/gcc.map > > @Maxim: would it be possible to update your script so that it will use: > --authors-file=gcc.map ? Should not be a problem. I'll try that. > > Is it desired for the transition to use the author map? Do we want it? IIUC, the downside is that converted repo will not match current git mirror unless we do log re-writing, which would add extra info on the side. -- Maxim Kuvyrkov www.linaro.org
Re: [Contrib PATCH] Add scripts to convert GCC repo from SVN to Git
> On May 17, 2019, at 2:06 AM, Joseph Myers wrote: > > On Tue, 14 May 2019, Maxim Kuvyrkov wrote: > >> The scripts convert svn history branch by branch. They rely on git-svn >> on convert individual branches. Git-svn is a good tool for converting >> individual branches. It is, however, either very slow at converting the >> entire GCC repo, or goes into infinite loop. > > I think git-svn is in fact a bad tool for repository conversion when the > history is nontrivial (for the reasons that have been discussed at length > in the past), I agree with this. However, with git -- we don't need to force ourselves to convert the whole history in one go; git-svn seems to be doing a good job at converting branch at a time. > and we should convert with reposurgeon. If reposurgeon works -- great, let's convert with it. It's good to have two independent tools so that we can compare and sanity check the results; from the top of my head: 1. number of merges should match on all branches, 2. changed files should match for all revisions. What I want to avoid is delaying the switch to git. > > ESR, can you give an update on the status of the conversion with > reposurgeon? You said "another serious attack on the repository > conversion is probably about two months out" in > <https://gcc.gnu.org/ml/gcc/2018-12/msg00112.html>. Is it on target to be > done by the time of the GNU Tools Cauldron in Montreal in September? > > And, could you bring git://thyrsus.com/repositories/gcc-conversion.git up > to date with changes since Jan 2018, or push the latest version of that > repository to some other public hosting location? That repository > represents what I consider the collaboratively built consensus on such > things as the desired author map (including handling of the ambiguous > author name), which directories represent branches and tags, and what tags > should be kept or removed - but building up such a consensus and keeping > it up to date over time (for new committers etc.) requires that the public > repository actually reflects the latest version of the conversion > machinery, day by day as the consensus develops. Review of that > repository will be important for reviewing the details of whether the > conversion is being done as desired - the details of the machinery will > help suggest things to spot-check in a converted repository. -- Maxim Kuvyrkov www.linaro.org
Re: [Contrib PATCH] Add scripts to convert GCC repo from SVN to Git
> On May 16, 2019, at 7:22 PM, Jeff Law wrote: > > On 5/15/19 5:19 AM, Richard Biener wrote: >> >> For the official converted repo do we really want all (old) >> development branches to be in the >> main git repo? I suppose we could create a readonly git from the >> state of the whole repository >> at the point of conversion (and also keep the SVN in readonly mode), >> just to make migration >> of content we want easy in the future? > I've always assumed we'd keep the old SVN tree read-only for historical > purposes. I strongly suspect that, ignoring release branches, that > non-active branches just aren't terribly interesting. Let's avoid mixing the two discussions: (1) converting svn repo to git (and getting community consensus to switch to git) and (2) deciding on which branches to keep in the new repo. With git, we can always split away unneeded history by removing unnecessary branches and tags and re-packing the repo. We can equally easily bring that history back if we change our minds. -- Maxim Kuvyrkov www.linaro.org
Re: [Contrib PATCH] Add scripts to convert GCC repo from SVN to Git
> On May 16, 2019, at 3:33 AM, Paul Koning wrote: > > > >> On May 15, 2019, at 2:42 PM, Eric Gallager wrote: >> >>> ... >> >> Wasn't Eric S. Raymond working on his own conversion of the GCC repo >> from SVN to Git? Whatever happened to his? > > Yes, and from what I recall he found that doing it fully correctly is an > extremely hard task. It might be a good idea to ask him to comment. That's a good suggestion; thanks, Paul. Hi Eric, The svn->git conversion scripts I'm testing work on individual svn branches, and I would appreciate a list of svn branches in GCC's repo that caused problems. It would be best to double-check conversion of these branches for any artifacts. Regards, -- Maxim Kuvyrkov www.linaro.org
