[Patch ifcvt] Add a new parameter to limit if-conversion

[Patch ifcvt] Add a new parameter to limit if-conversion

[James Greenhalgh]

[-- Attachment #1: Type: text/plain, Size: 1006 bytes --]


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  <james.greenhalgh@arm.com>

	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  <james.greenhalgh@arm.com>

	PR rtl-optimization/68920
	* gcc.deg/ifcvt-5.c: New.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Patch-ifcvt-Add-a-new-parameter-to-limit-if-conversi.patch --]
[-- Type: text/x-patch;  name=0001-Patch-ifcvt-Add-a-new-parameter-to-limit-if-conversi.patch, Size: 3191 bytes --]

diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 9b3e2fe..1f9b0fe 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -10341,6 +10341,14 @@ In each case, the @var{value} is an integer.  The allowable choices for
 When branch is predicted to be taken with probability lower than this threshold
 (in percent), then it is considered well predictable. The default is 10.
 
+@item max-rtl-if-conversion-insns
+RTL if-conversion tries to remove conditional branches around a block and
+replace them with conditionally executed instructions.  This parameter
+gives the maximum number of instructions in a block which should be
+considered for if-conversion.  The default is 10, though the compiler will
+also use other heuristics to decide whether if-conversion is likely to be
+profitable.
+
 @item max-crossjump-edges
 The maximum number of incoming edges to consider for cross-jumping.
 The algorithm used by @option{-fcrossjumping} is @math{O(N^2)} in
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 6164232..2674baa 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -32,6 +32,7 @@
 #include "regs.h"
 #include "emit-rtl.h"
 #include "recog.h"
+#include "params.h"
 
 #include "cfgrtl.h"
 #include "cfganal.h"
@@ -3944,6 +3945,9 @@ noce_find_if_block (basic_block test_bb, edge then_edge, edge else_edge,
   if_info.then_else_reversed = then_else_reversed;
   if_info.branch_cost = BRANCH_COST (optimize_bb_for_speed_p (test_bb),
 				     predictable_edge_p (then_edge));
+  if_info.branch_cost
+    = MIN (if_info.branch_cost,
+	   (unsigned int) PARAM_VALUE (PARAM_MAX_RTL_IF_CONVERSION_INSNS));
 
   /* Do the real work.  */
 
diff --git a/gcc/params.def b/gcc/params.def
index 41fd8a8..a0d9787 100644
--- a/gcc/params.def
+++ b/gcc/params.def
@@ -49,6 +49,15 @@ DEFPARAM (PARAM_PREDICTABLE_BRANCH_OUTCOME,
 	  "Maximal estimated outcome of branch considered predictable.",
 	  2, 0, 50)
 
+/* The maximum number of insns in a basic block to consider for
+   RTL if-conversion.  A target might use BRANCH_COST to further limit the
+   number of insns considered.  */
+DEFPARAM (PARAM_MAX_RTL_IF_CONVERSION_INSNS,
+	  "max-rtl-if-conversion-insns",
+	  "Maximum number of insns in a basic block to consider for RTL "
+	  "if-conversion.",
+	  10, 0, 10)
+
 DEFPARAM (PARAM_INLINE_MIN_SPEEDUP,
 	  "inline-min-speedup",
 	  "The minimal estimated speedup allowing inliner to ignore inline-insns-single and inline-isnsns-auto.",
diff --git a/gcc/testsuite/gcc.dg/ifcvt-5.c b/gcc/testsuite/gcc.dg/ifcvt-5.c
new file mode 100644
index 0000000..fc6a2ca
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ifcvt-5.c
@@ -0,0 +1,19 @@
+/* Check that multi-insn if-conversion is not done if the override
+   parameter would not allow it.  */
+
+/* { dg-options "-fdump-rtl-ce1 -O2 --param max-rtl-if-conversion-insns=1" } */
+int
+foo (int x, int y, int a)
+{
+  int i = x;
+  int j = y;
+  /* Try to make taking the branch likely.  */
+  __builtin_expect (x > y, 1);
+  if (x > y)
+    {
+      i = a;
+      j = i;
+    }
+  return i * j;
+}
+/* { dg-final { scan-rtl-dump "0 true changes made" "ce1" } } */

Re: [Patch ifcvt] Add a new parameter to limit if-conversion

[Yuri Rumyantsev]

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 <james.greenhalgh@arm.com>:
>
> 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  <james.greenhalgh@arm.com>
>
>         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  <james.greenhalgh@arm.com>
>
>         PR rtl-optimization/68920
>         * gcc.deg/ifcvt-5.c: New.
>

Re: [Patch ifcvt] Add a new parameter to limit if-conversion

[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 <james.greenhalgh@arm.com>:
> >
> > 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  <james.greenhalgh@arm.com>
> >
> >         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  <james.greenhalgh@arm.com>
> >
> >         PR rtl-optimization/68920
> >         * gcc.deg/ifcvt-5.c: New.
> >
> 

Re: [Patch ifcvt] Add a new parameter to limit if-conversion

[Yuri Rumyantsev]

[-- Attachment #1: Type: text/plain, Size: 3766 bytes --]

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  <ysrumyan@gmail.com>

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 <james.greenhalgh@arm.com>:
> 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 <james.greenhalgh@arm.com>:
>> >
>> > 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  <james.greenhalgh@arm.com>
>> >
>> >         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  <james.greenhalgh@arm.com>
>> >
>> >         PR rtl-optimization/68920
>> >         * gcc.deg/ifcvt-5.c: New.
>> >
>>

[-- Attachment #2: patch --]
[-- Type: application/octet-stream, Size: 5687 bytes --]

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index 733d0ab..f04d12e 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -5343,6 +5343,13 @@ ix86_option_override_internal (bool main_args_p,
 			 opts->x_param_values,
 			 opts_set->x_param_values);
 
+  /* Restrict number of if-converted SET insns to 1.  */
+  if (TARGET_ONE_IF_CONV_INSN)
+    maybe_set_param_value (PARAM_MAX_RTL_IF_CONVERSION_INSNS,
+			   1,
+			   opts->x_param_values,
+			   opts_set->x_param_values);
+
   /* Enable sw prefetching at -O3 for CPUS that prefetching is helpful.  */
   if (opts->x_flag_prefetch_loop_arrays < 0
       && HAVE_prefetch
diff --git a/gcc/config/i386/i386.h b/gcc/config/i386/i386.h
index 7e6548b..9100a803 100644
--- a/gcc/config/i386/i386.h
+++ b/gcc/config/i386/i386.h
@@ -499,6 +499,8 @@ extern unsigned char ix86_tune_features[X86_TUNE_LAST];
     ix86_tune_features[X86_TUNE_ADJUST_UNROLL]
 #define TARGET_AVOID_FALSE_DEP_FOR_BMI \
 	ix86_tune_features[X86_TUNE_AVOID_FALSE_DEP_FOR_BMI]
+#define TARGET_ONE_IF_CONV_INSN \
+	ix86_tune_features[X86_TUNE_ONE_IF_CONV_INSN]
 
 /* Feature tests against the various architecture variations.  */
 enum ix86_arch_indices {
diff --git a/gcc/config/i386/x86-tune.def b/gcc/config/i386/x86-tune.def
index b2d3921..f7f8842 100644
--- a/gcc/config/i386/x86-tune.def
+++ b/gcc/config/i386/x86-tune.def
@@ -550,3 +550,8 @@ DEF_TUNE (X86_TUNE_PROMOTE_QI_REGS, "promote_qi_regs", 0)
    unrolling small loop less important. For, such architectures we adjust
    the unroll factor so that the unrolled loop fits the loop buffer.  */
 DEF_TUNE (X86_TUNE_ADJUST_UNROLL, "adjust_unroll_factor", m_BDVER3 | m_BDVER4)
+
+/* X86_TUNE_ONE_IF_CONV_INSNS: Restrict a number of set insns to be
+   if-converted to one.  */
+DEF_TUNE (X86_TUNE_ONE_IF_CONV_INSN, "one_if_conv_insn",
+	  m_SILVERMONT | m_KNL | m_INTEL | m_CORE_ALL | m_GENERIC)
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index 4e2cf8f..73c3a29 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -10341,6 +10341,14 @@ In each case, the @var{value} is an integer.  The allowable choices for
 When branch is predicted to be taken with probability lower than this threshold
 (in percent), then it is considered well predictable. The default is 10.
 
+@item max-rtl-if-conversion-insns
+RTL if-conversion tries to remove conditional branches around a block and
+replace them with conditionally executed instructions.  This parameter
+gives the maximum number of instructions in a block which should be
+considered for if-conversion.  The default is 10, though the compiler will
+also use other heuristics to decide whether if-conversion is likely to be
+profitable.
+
 @item max-crossjump-edges
 The maximum number of incoming edges to consider for cross-jumping.
 The algorithm used by @option{-fcrossjumping} is @math{O(N^2)} in
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 6164232..189d60f 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -44,6 +44,7 @@
 #include "shrink-wrap.h"
 #include "rtl-iter.h"
 #include "ifcvt.h"
+#include "params.h"
 
 #ifndef MAX_CONDITIONAL_EXECUTE
 #define MAX_CONDITIONAL_EXECUTE \
@@ -3242,6 +3243,8 @@ bb_ok_for_noce_convert_multiple_sets (basic_block test_bb,
 {
   rtx_insn *insn;
   unsigned count = 0;
+  unsigned param = PARAM_VALUE (PARAM_MAX_RTL_IF_CONVERSION_INSNS);
+  unsigned limit = MIN (ii->branch_cost, param);
 
   FOR_BB_INSNS (test_bb, insn)
     {
@@ -3277,8 +3280,8 @@ bb_ok_for_noce_convert_multiple_sets (basic_block test_bb,
   /* FORNOW: Our cost model is a count of the number of instructions we
      would if-convert.  This is suboptimal, and should be improved as part
      of a wider rework of branch_cost.  */
-  if (count > ii->branch_cost)
-    return FALSE;
+  if (count > limit)
+    return false;
 
   return count > 0;
 }
@@ -3823,7 +3826,6 @@ cond_move_process_if_block (struct noce_if_info *if_info)
     }
 
   num_updated_if_blocks++;
-
   success_p = TRUE;
 
 done:
@@ -4807,7 +4809,6 @@ find_if_case_1 (basic_block test_bb, edge then_edge, edge else_edge)
 
   num_true_changes++;
   num_updated_if_blocks++;
-
   return TRUE;
 }
 
diff --git a/gcc/params.def b/gcc/params.def
index 9b82164..f168066 100644
--- a/gcc/params.def
+++ b/gcc/params.def
@@ -1177,6 +1177,12 @@ DEFPARAM (PARAM_MAX_SSA_NAME_QUERY_DEPTH,
 	  "Maximum recursion depth allowed when querying a property of an"
 	  " SSA name.",
 	  2, 1, 0)
+
+DEFPARAM (PARAM_MAX_RTL_IF_CONVERSION_INSNS,
+	  "max-rtl-if-conversion-insns",
+	  "Maximum number of insns in a basic block to consider for RTL "
+	  "if-conversion.",
+	  10, 0, 99)
 /*
 
 Local variables:
diff --git a/gcc/testsuite/gcc.dg/ifcvt-4.c b/gcc/testsuite/gcc.dg/ifcvt-4.c
index e1c81fb..9a91a19 100644
--- a/gcc/testsuite/gcc.dg/ifcvt-4.c
+++ b/gcc/testsuite/gcc.dg/ifcvt-4.c
@@ -1,4 +1,4 @@
-/* { dg-options "-fdump-rtl-ce1 -O2" } */
+/* { dg-options "-fdump-rtl-ce1 -O2 --param max-rtl-if-conversion-insns=3" } */
 /* { dg-skip-if "Multiple set if-conversion not guaranteed on all subtargets" { "arm*-*-* powerpc64le*-*-*" } {"*"} { "" } }  */
 
 int
diff --git a/gcc/testsuite/gcc.dg/ifcvt-5.c b/gcc/testsuite/gcc.dg/ifcvt-5.c
new file mode 100755
index 0000000..0b73e54
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/ifcvt-5.c
@@ -0,0 +1,17 @@
+/* Check that multi-insn if-conversion is not done if the override
+   parameter would not allow it.  */
+
+/* { dg-options "-fdump-rtl-ce1 -O2 --param max-rtl-if-conversion-insns=1" } */
+int
+foo (int x, int y, int a)
+{
+  int i = x;
+  int j = y;
+  if (x > y)
+    {
+      i = a;
+      j = i;
+    }
+  return i * j;
+}
+/* { dg-final { scan-rtl-dump "0 true changes made" "ce1" } } */

Re: [Patch ifcvt] Add a new parameter to limit if-conversion

[Bernd Schmidt]

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

[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, schwab@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

[Yuri Rumyantsev]

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 <schwab@suse.de>:
> 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, schwab@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 Schwab]

Yuri Rumyantsev <ysrumyan@gmail.com> writes:

> Is it OK for you if we exclude dg/ifcvt-5.c from ia64 testing

Sure, go ahead.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@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

[Yuri Rumyantsev]

[-- Attachment #1: Type: text/plain, Size: 653 bytes --]

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  <ysrumyan@gmail.com>

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 <schwab@suse.de>:
> Yuri Rumyantsev <ysrumyan@gmail.com> writes:
>
>> Is it OK for you if we exclude dg/ifcvt-5.c from ia64 testing
>
> Sure, go ahead.
>
> Andreas.
>
> --
> Andreas Schwab, SUSE Labs, schwab@suse.de
> GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
> "And now for something completely different."

[-- Attachment #2: patch.1 --]
[-- Type: application/octet-stream, Size: 492 bytes --]

diff --git a/gcc/testsuite/gcc.dg/ifcvt-5.c b/gcc/testsuite/gcc.dg/ifcvt-5.c
old mode 100755
new mode 100644
index 0b73e54..59f4b5f
--- a/gcc/testsuite/gcc.dg/ifcvt-5.c
+++ b/gcc/testsuite/gcc.dg/ifcvt-5.c
@@ -2,6 +2,8 @@
    parameter would not allow it.  */
 
 /* { dg-options "-fdump-rtl-ce1 -O2 --param max-rtl-if-conversion-insns=1" } */
+/* { dg-skip-if "Multi-insn if-conversion is always done on some subtargets" { "ia64-*-*" } {"*"} { "" } }  */
+
 int
 foo (int x, int y, int a)
 {

Re: [Patch ifcvt] Add a new parameter to limit if-conversion

[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

[Yuri Rumyantsev]

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<rtx> 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  <ysrumyan@gmail.com>

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 <bschmidt@redhat.com>:
> 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

[Bernd Schmidt]

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