Re: [Patch ifcvt] Add a new parameter to limit if-conversion
PR rtl-optimization/68920 * ifcvt.c (cond_move_process_if_block): Limit number of conditional moves. Ok. This should probably be consolidated a bit post 6.0. Bernd
Re: [Patch ifcvt] Add a new parameter to limit if-conversion
Hi Bernd, Here is updated patch as your proposed to avoid regression on ia64. Bootstarp and regression testing on x86_64 did not show any new failures. Is it OK for trunk? Thanks. diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c index 189d60f..ef6fa69 100644 --- a/gcc/ifcvt.c +++ b/gcc/ifcvt.c @@ -3730,6 +3730,7 @@ cond_move_process_if_block (struct noce_if_info *if_info) vec else_regs = vNULL; unsigned int i; int success_p = FALSE; + int limit = PARAM_VALUE (PARAM_MAX_RTL_IF_CONVERSION_INSNS); /* Build a mapping for each block to the value used for each register. */ @@ -3779,7 +3780,8 @@ cond_move_process_if_block (struct noce_if_info *if_info) is the number of assignments currently made in only one of the branches, since if we convert we are going to always execute them. */ - if (c > MAX_CONDITIONAL_EXECUTE) + if (c > MAX_CONDITIONAL_EXECUTE + || c > limit) goto done; /* Try to emit the conditional moves. First do the then block, ChangeLog: 2016-01-13 Yuri Rumyantsev PR rtl-optimization/68920 * ifcvt.c (cond_move_process_if_block): Limit number of conditional moves. 2016-01-13 4:52 GMT+03:00 Bernd Schmidt : > On 01/12/2016 04:41 PM, Yuri Rumyantsev wrote: >> >> Here is a simple fix to exclude dg/ifcvt-5.c test from ia64 testing. >> >> Is it OK for trunk? > > > Please ensure patches are attached as plain text so that reviewers don't > have to save them to be able to read them. > > It looks like ia64 is making chanes in cond_move_process_if_block. Maybe > that function needs to take the param into account so we don't have to keep > adding excluded targets to the testcase? > > > Bernd >
Re: [Patch ifcvt] Add a new parameter to limit if-conversion
On 01/12/2016 04:41 PM, Yuri Rumyantsev wrote: Here is a simple fix to exclude dg/ifcvt-5.c test from ia64 testing. Is it OK for trunk? Please ensure patches are attached as plain text so that reviewers don't have to save them to be able to read them. It looks like ia64 is making chanes in cond_move_process_if_block. Maybe that function needs to take the param into account so we don't have to keep adding excluded targets to the testcase? Bernd
Re: [Patch ifcvt] Add a new parameter to limit if-conversion
Hi All, Here is a simple fix to exclude dg/ifcvt-5.c test from ia64 testing. Is it OK for trunk? testsuite/ChangeLog: 2016-01-12 Yuri Rumyantsev PR rtl-optimization/68920 gcc/testsuite/ChangeLog * gcc.dg/ifcvt-5.c: Exclude it from ia64 testing. 2016-01-12 17:01 GMT+03:00 Andreas Schwab : > Yuri Rumyantsev writes: > >> Is it OK for you if we exclude dg/ifcvt-5.c from ia64 testing > > Sure, go ahead. > > Andreas. > > -- > Andreas Schwab, SUSE Labs, sch...@suse.de > GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 > "And now for something completely different." patch.1 Description: Binary data
Re: [Patch ifcvt] Add a new parameter to limit if-conversion
Yuri Rumyantsev writes: > Is it OK for you if we exclude dg/ifcvt-5.c from ia64 testing Sure, go ahead. Andreas. -- Andreas Schwab, SUSE Labs, sch...@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
Re: [Patch ifcvt] Add a new parameter to limit if-conversion
Andreas, Is it OK for you if we exclude dg/ifcvt-5.c from ia64 testing since predication must be used instead of conditional move's. 2016-01-12 13:07 GMT+03:00 Andreas Schwab : > gcc.dg/ifcvt-5.c fails on ia64: > > From ifcvt-5.c.223r.ce1: > > == Pass 2 == > > > == no more changes > > 1 possible IF blocks searched. > 1 IF blocks converted. > 2 true changes made. > > Andreas. > > -- > Andreas Schwab, SUSE Labs, sch...@suse.de > GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 > "And now for something completely different."
Re: [Patch ifcvt] Add a new parameter to limit if-conversion
gcc.dg/ifcvt-5.c fails on ia64: >From ifcvt-5.c.223r.ce1: == Pass 2 == == no more changes 1 possible IF blocks searched. 1 IF blocks converted. 2 true changes made. Andreas. -- Andreas Schwab, SUSE Labs, sch...@suse.de GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7 "And now for something completely different."
Re: [Patch ifcvt] Add a new parameter to limit if-conversion
On 12/31/2015 10:21 AM, Yuri Rumyantsev wrote: Here is slightly modified patch which limits a number of conditional moves instead of changing conditional branch cost. This is in fact a work-around for very poor cost model which needs to be enhanced to evaluate cost of conditional move that could be greater then cost of ordinary move (for some targets). This fix did not show any performance regressions on different x86 platforms in comparison with James patch. I think this is OK. In the future, when attaching patches, please make sure they are text/plain so they are displayed by mail readers and can be quoted. Bernd
Re: [Patch ifcvt] Add a new parameter to limit if-conversion
Hi All, Here is slightly modified patch which limits a number of conditional moves instead of changing conditional branch cost. This is in fact a work-around for very poor cost model which needs to be enhanced to evaluate cost of conditional move that could be greater then cost of ordinary move (for some targets). This fix did not show any performance regressions on different x86 platforms in comparison with James patch. Bootstrap and regression testing did not show any new failures. Is it OK for trunk? ChangeLog: 2015-12-31 Yuri Rumyantsev PR rtl-optimization/68920 * config/i386/i386.c (ix86_option_override_internal): Restrict number of conditional moves for RTL if-conversion to 1 for TARGET_ONE_IF_CONV_INSN. * config/i386/i386.h (TARGET_ONE_IF_CONV_INSN): New macros. * config/i386/x86-tune.def (X86_TUNE_ONE_IF_CONV_INSN): New macros. * params.def (PARAM_MAX_RTL_IF_CONVERSION_INSNS) : Introduce new parameter to restirct number of conditional moves for RTL if-conversion. * doc/invoke.texi (max-rtl-if-conversion-insns): Document it. * ifcvt.c (bb_ok_for_noce_convert_multiple_sets): Limit number of conditionl moves. gcc/testsuite/ChangeLog * gcc.dg/ifcvt-4.c: Add "--param max-rtl-if-conversion-insns=3" option for ix86 targets. * gcc.dg/ifcvt-5.c: New test. 2015-12-18 17:19 GMT+03:00 James Greenhalgh : > On Fri, Dec 18, 2015 at 05:09:14PM +0300, Yuri Rumyantsev wrote: >> James, >> >> We implemented slightly different patch - we restrict number of SET >> instructions for if-conversion through new parameter and add check in >> bb_ok_for_noce_convert_multiple_sets: >> >> + unsigned limit = MIN (ii->branch_cost, >> + (unsigned) PARAM_VALUE (PARAM_MAX_IF_CONV_SET_INSNS)); >> .. >> - if (count > ii->branch_cost) >> -return FALSE; >> + if (count > limit) >> +return false; >> >> Now we are testing it for different suites/chips but I saw that real >> benchmark for which we saw huge performance degradation gets speed-ip >> on 65% for -march=slm -m32. > > Yes, that will work too. I put it where I did to give back-ends the choice > to turn off all if-conversion by setting the param to zero. Your fix is more > targeted to fixing just the one regression. I don't have a preference as > to which direction we take this. I saw a similar performance boost for your > testcase on my development box with my patch - so at least we now have two > candidate patches to fix the performance regression. > > Thanks, > James > >> >> 2015-12-18 16:52 GMT+03:00 James Greenhalgh : >> > >> > Hi, >> > >> > PR68920 talks about undesirable if-conversion in the x86 back-end. >> > The if-conversion cost model simply uses BRANCH_COST (I want to revisit >> > this for GCC 7), but BRANCH_COST is overloaded in the compiler and reducing >> > it causes other optimisations to be disabled. >> > >> > Consequently, to fix the PR we want something new that the target can set >> > to override BRANCH_COST and reduce the number of instructions that get >> > if-converted. This patch adds this mechanism through a param. >> > >> > Bootstrapped on x86_64-none-linux-gnu and aarch64-none-linux-gnu. >> > >> > OK for trunk? >> > >> > Thanks, >> > James >> > >> > --- >> > gcc/ >> > >> > 2015-12-17 James Greenhalgh >> > >> > PR rtl-optimization/68920 >> > * params.def (PARAM_MAX_RTL_IF_CONVERSION_INSNS): New. >> > * ifcvt.c (noce_find_if_block): Limit by new param. >> > * doc/invoke.texi (max-rtl-if-conversion-insns): Document it. >> > >> > gcc/testsuite/ >> > >> > 2015-12-17 James Greenhalgh >> > >> > PR rtl-optimization/68920 >> > * gcc.deg/ifcvt-5.c: New. >> > >> patch Description: Binary data
Re: [Patch ifcvt] Add a new parameter to limit if-conversion
On Fri, Dec 18, 2015 at 05:09:14PM +0300, Yuri Rumyantsev wrote: > James, > > We implemented slightly different patch - we restrict number of SET > instructions for if-conversion through new parameter and add check in > bb_ok_for_noce_convert_multiple_sets: > > + unsigned limit = MIN (ii->branch_cost, > + (unsigned) PARAM_VALUE (PARAM_MAX_IF_CONV_SET_INSNS)); > .. > - if (count > ii->branch_cost) > -return FALSE; > + if (count > limit) > +return false; > > Now we are testing it for different suites/chips but I saw that real > benchmark for which we saw huge performance degradation gets speed-ip > on 65% for -march=slm -m32. Yes, that will work too. I put it where I did to give back-ends the choice to turn off all if-conversion by setting the param to zero. Your fix is more targeted to fixing just the one regression. I don't have a preference as to which direction we take this. I saw a similar performance boost for your testcase on my development box with my patch - so at least we now have two candidate patches to fix the performance regression. Thanks, James > > 2015-12-18 16:52 GMT+03:00 James Greenhalgh : > > > > Hi, > > > > PR68920 talks about undesirable if-conversion in the x86 back-end. > > The if-conversion cost model simply uses BRANCH_COST (I want to revisit > > this for GCC 7), but BRANCH_COST is overloaded in the compiler and reducing > > it causes other optimisations to be disabled. > > > > Consequently, to fix the PR we want something new that the target can set > > to override BRANCH_COST and reduce the number of instructions that get > > if-converted. This patch adds this mechanism through a param. > > > > Bootstrapped on x86_64-none-linux-gnu and aarch64-none-linux-gnu. > > > > OK for trunk? > > > > Thanks, > > James > > > > --- > > gcc/ > > > > 2015-12-17 James Greenhalgh > > > > PR rtl-optimization/68920 > > * params.def (PARAM_MAX_RTL_IF_CONVERSION_INSNS): New. > > * ifcvt.c (noce_find_if_block): Limit by new param. > > * doc/invoke.texi (max-rtl-if-conversion-insns): Document it. > > > > gcc/testsuite/ > > > > 2015-12-17 James Greenhalgh > > > > PR rtl-optimization/68920 > > * gcc.deg/ifcvt-5.c: New. > > >
Re: [Patch ifcvt] Add a new parameter to limit if-conversion
James, We implemented slightly different patch - we restrict number of SET instructions for if-conversion through new parameter and add check in bb_ok_for_noce_convert_multiple_sets: + unsigned limit = MIN (ii->branch_cost, + (unsigned) PARAM_VALUE (PARAM_MAX_IF_CONV_SET_INSNS)); .. - if (count > ii->branch_cost) -return FALSE; + if (count > limit) +return false; Now we are testing it for different suites/chips but I saw that real benchmark for which we saw huge performance degradation gets speed-ip on 65% for -march=slm -m32. 2015-12-18 16:52 GMT+03:00 James Greenhalgh : > > Hi, > > PR68920 talks about undesirable if-conversion in the x86 back-end. > The if-conversion cost model simply uses BRANCH_COST (I want to revisit > this for GCC 7), but BRANCH_COST is overloaded in the compiler and reducing > it causes other optimisations to be disabled. > > Consequently, to fix the PR we want something new that the target can set > to override BRANCH_COST and reduce the number of instructions that get > if-converted. This patch adds this mechanism through a param. > > Bootstrapped on x86_64-none-linux-gnu and aarch64-none-linux-gnu. > > OK for trunk? > > Thanks, > James > > --- > gcc/ > > 2015-12-17 James Greenhalgh > > PR rtl-optimization/68920 > * params.def (PARAM_MAX_RTL_IF_CONVERSION_INSNS): New. > * ifcvt.c (noce_find_if_block): Limit by new param. > * doc/invoke.texi (max-rtl-if-conversion-insns): Document it. > > gcc/testsuite/ > > 2015-12-17 James Greenhalgh > > PR rtl-optimization/68920 > * gcc.deg/ifcvt-5.c: New. >
