Re: Re: ira.c update_equiv_regs patch causes gcc/testsuite/gcc.target/arm/pr43920-2.c regression
On 21/04/15 06:27, Jeff Law wrote: On 04/20/2015 01:09 AM, Shiva Chen wrote: Hi, Jeff Thanks for your advice. can_replace_by.patch is the new patch to handle both cases. pr43920-2.c.244r.jump2.ori is the original jump2 rtl dump pr43920-2.c.244r.jump2.patch_can_replace_by is the jump2 rtl dump after patch can_replace_by.patch Could you help me to review the patch? Thanks. This looks pretty good. I expanded the comment for the new function a bit and renamed the function in an effort to clarify its purpose. From reviewing can_replace_by, it seems it should have been handling this case, but clearly wasn't due to implementation details. I then bootstrapped and regression tested the patch on x86_64-linux-gnu where it passed. I also instrumented that compiler to see how often this code triggers. During a bootstrap it triggers a couple hundred times (which is obviously a proxy for cross jumping improvements). So it's triggering regularly on x86_64, which is good. I also verified that this fixes BZ64916 for an arm-non-eabi toolchain configured with --with-arch=armv7. Installed on the trunk. No new testcase as it's covered by existing tests. Thanks,, jeff Hi, I see this patch been committed in r56 on trunk. Is it okay to port this to fsf-5? Kind regards, Alex
Re: ira.c update_equiv_regs patch causes gcc/testsuite/gcc.target/arm/pr43920-2.c regression
On 29/07/15 23:14, Jeff Law wrote: On 07/28/2015 12:18 PM, Alex Velenko wrote: On 21/04/15 06:27, Jeff Law wrote: On 04/20/2015 01:09 AM, Shiva Chen wrote: Hi, Jeff Thanks for your advice. can_replace_by.patch is the new patch to handle both cases. pr43920-2.c.244r.jump2.ori is the original jump2 rtl dump pr43920-2.c.244r.jump2.patch_can_replace_by is the jump2 rtl dump after patch can_replace_by.patch Could you help me to review the patch? Thanks. This looks pretty good. I expanded the comment for the new function a bit and renamed the function in an effort to clarify its purpose. From reviewing can_replace_by, it seems it should have been handling this case, but clearly wasn't due to implementation details. I then bootstrapped and regression tested the patch on x86_64-linux-gnu where it passed. I also instrumented that compiler to see how often this code triggers. During a bootstrap it triggers a couple hundred times (which is obviously a proxy for cross jumping improvements). So it's triggering regularly on x86_64, which is good. I also verified that this fixes BZ64916 for an arm-non-eabi toolchain configured with --with-arch=armv7. Installed on the trunk. No new testcase as it's covered by existing tests. Thanks,, jeff Hi, I see this patch been committed in r56 on trunk. Is it okay to port this to fsf-5? It's not a regression, so backporting it would be generally frowned upon. If you feel strongly about it, you should ask Jakub, Joseph or Richi (the release managers) for an exception to the general policy. jeff Hi Jakub, Can this commit be ported to fsf-5? It fixed gcc.target/arm/pr43920-2.c at the time, so I think it is a good idea to port. Please, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64916 Kind regards, Alex
Re: ira.c update_equiv_regs patch causes gcc/testsuite/gcc.target/arm/pr43920-2.c regression
On 31/07/15 12:04, Alex Velenko wrote: On 29/07/15 23:14, Jeff Law wrote: On 07/28/2015 12:18 PM, Alex Velenko wrote: On 21/04/15 06:27, Jeff Law wrote: On 04/20/2015 01:09 AM, Shiva Chen wrote: Hi, Jeff Thanks for your advice. can_replace_by.patch is the new patch to handle both cases. pr43920-2.c.244r.jump2.ori is the original jump2 rtl dump pr43920-2.c.244r.jump2.patch_can_replace_by is the jump2 rtl dump after patch can_replace_by.patch Could you help me to review the patch? Thanks. This looks pretty good. I expanded the comment for the new function a bit and renamed the function in an effort to clarify its purpose. From reviewing can_replace_by, it seems it should have been handling this case, but clearly wasn't due to implementation details. I then bootstrapped and regression tested the patch on x86_64-linux-gnu where it passed. I also instrumented that compiler to see how often this code triggers. During a bootstrap it triggers a couple hundred times (which is obviously a proxy for cross jumping improvements). So it's triggering regularly on x86_64, which is good. I also verified that this fixes BZ64916 for an arm-non-eabi toolchain configured with --with-arch=armv7. Installed on the trunk. No new testcase as it's covered by existing tests. Thanks,, jeff Hi, I see this patch been committed in r56 on trunk. Is it okay to port this to fsf-5? It's not a regression, so backporting it would be generally frowned upon. If you feel strongly about it, you should ask Jakub, Joseph or Richi (the release managers) for an exception to the general policy. jeff Hi Jakub, Can this commit be ported to fsf-5? It fixed gcc.target/arm/pr43920-2.c at the time, so I think it is a good idea to port. Please, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64916 Kind regards, Alex Ping! Currently this test is passed on fsf-trunk, but not passed on fsf-5, so I think it is a regression on fsf-5: arm-none-eabi fsf-5: PASS: gcc.target/arm/pr43920-2.c (test for excess errors) FAIL: gcc.target/arm/pr43920-2.c scan-assembler-times pop 2 PASS: gcc.target/arm/pr43920-2.c scan-assembler-times beq 3 Executing on host: arm-none-eabi-size pr43920-2.o (timeout = 300) spawn arm-none-eabi-size pr43920-2.o text databssdechexfilename 58 0 0 58 3apr43920-2.o text size is 58 FAIL: gcc.target/arm/pr43920-2.c object-size text <= 54 arm-none-eabi fsf-trunk: PASS: gcc.target/arm/pr43920-2.c (test for excess errors) PASS: gcc.target/arm/pr43920-2.c scan-assembler-times pop 2 PASS: gcc.target/arm/pr43920-2.c scan-assembler-times beq 3 size is arm-none-eabi-size Executing on host: arm-none-eabi-size pr43920-2.o (timeout = 300) spawn arm-none-eabi-size pr43920-2.o text databssdechexfilename 54 0 0 54 36pr43920-2.o text size is 54 PASS: gcc.target/arm/pr43920-2.c object-size text <= 54 Can this, please, be ported? Kind regards, Alex
Re: ira.c update_equiv_regs patch causes gcc/testsuite/gcc.target/arm/pr43920-2.c regression
On 18/08/15 10:45, Marcus Shawcroft wrote: On 18 August 2015 at 10:25, Alex Velenko wrote: On 31/07/15 12:04, Alex Velenko wrote: On 29/07/15 23:14, Jeff Law wrote: On 07/28/2015 12:18 PM, Alex Velenko wrote: On 21/04/15 06:27, Jeff Law wrote: On 04/20/2015 01:09 AM, Shiva Chen wrote: Hi, Jeff Thanks for your advice. can_replace_by.patch is the new patch to handle both cases. pr43920-2.c.244r.jump2.ori is the original jump2 rtl dump pr43920-2.c.244r.jump2.patch_can_replace_by is the jump2 rtl dump after patch can_replace_by.patch Could you help me to review the patch? Thanks. This looks pretty good. I expanded the comment for the new function a bit and renamed the function in an effort to clarify its purpose. From reviewing can_replace_by, it seems it should have been handling this case, but clearly wasn't due to implementation details. I then bootstrapped and regression tested the patch on x86_64-linux-gnu where it passed. I also instrumented that compiler to see how often this code triggers. During a bootstrap it triggers a couple hundred times (which is obviously a proxy for cross jumping improvements). So it's triggering regularly on x86_64, which is good. I also verified that this fixes BZ64916 for an arm-non-eabi toolchain configured with --with-arch=armv7. Installed on the trunk. No new testcase as it's covered by existing tests. Thanks,, jeff Hi, I see this patch been committed in r56 on trunk. Is it okay to port this to fsf-5? It's not a regression, so backporting it would be generally frowned upon. If you feel strongly about it, you should ask Jakub, Joseph or Richi (the release managers) for an exception to the general policy. jeff Hi Jakub, Can this commit be ported to fsf-5? It fixed gcc.target/arm/pr43920-2.c at the time, so I think it is a good idea to port. Please, see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64916 Kind regards, Alex Ping! Currently this test is passed on fsf-trunk, but not passed on fsf-5, so I think it is a regression on fsf-5: That does not make it a regression, it is only a regression if a version prior to 5 passes, how does this test behave on 4.9? Cheers /Marcus Hi Marcus, On fsf-4.9 I see the test pass: PASS: gcc.target/arm/pr43920-2.c (test for excess errors) PASS: gcc.target/arm/pr43920-2.c scan-assembler-times pop 2 PASS: gcc.target/arm/pr43920-2.c scan-assembler-times beq 3 Executing on host: arm-none-eabi-size pr43920-2.o (timeout = 300) spawn arm-none-eabi-size pr43920-2.o textdata bss dec hex filename 54 0 0 54 36 pr43920-2.o text size is 54 PASS: gcc.target/arm/pr43920-2.c object-size text <= 54 So this is a regression in fsf-5. Kind regards, Alex
