diff mbox

Add support on powerpc to change CASE_VALUES_THRESHOLD

Message ID 20110630170650.GA3801@hungry-tiger.westford.ibm.com
State New
Headers show

Commit Message

Michael Meissner June 30, 2011, 5:06 p.m. UTC
On Thu, Jun 30, 2011 at 12:31:44PM +0200, Richard Guenther wrote:
> On Thu, Jun 30, 2011 at 12:34 AM, Michael Meissner
> <meissner@linux.vnet.ibm.com> wrote:
> > On the powerpc, switch statements can be expensive, and we would like to be
> > able to tune the threshold of when the compiler generates if statements
> > vs. using a table jump operation (and different processors within the powerpc
> > have different limits).  This patch adds a powerpc tuning option to control
> > this.
> >
> > I've done bootstraps and make checks with no regressions.  Is this ok to apply
> > to the trunk?  At this time, I am not changing the default value (4).  With the
> > option, I've seen a few spec 2006 benchmarks run faster, and a few run slower.
> 
> Hmm.  This hook looks like it could be turned into a --param.  In fact
> it doesn't get whatever context information, so I wonder if any target
> does something fancy.

I've done it also as a --param.  I tend to think it is better as a param, and
then other people can tune their code.  One slight problem is the default for
CASE_VALUES_THRESHOLD depends on whether you have a named casesi pattern that
is active (HAVE_casesi ? 4 : 5).  Either we need two separate parameters, or we
have just one parameter, and if that is 0, fall back to the 4 or 5 value.

This patch swtiches to use --param to set the value.  Is it acceptable to check
in?  I've done a bootstrap and I'm about to do a make check.

[gcc]
2011-06-30  Michael Meissner  <meissner@linux.vnet.ibm.com>

	* params.def (PARAM_CASE_VALUES_THRESHOLD): New parameter to
	override CASE_VALUES_THRESHOLD.

	* stmt.c (toplevel): Include params.h.
	(case_values_threshold): Use the --param case-values-threshold
	value if non-zero, otherwise use machine dependent value.
	(expand_case): Use case_values_threshold.

	* Makefile.in (stmt.o): Add $(PARAMS_H) dependency.

	* doc/invoke.texi (--param case-values-threshold): Document.

[gcc/testsuite]
2011-06-30  Michael Meissner  <meissner@linux.vnet.ibm.com>

	* gcc.target/powerpc/ppc-switch-1.c: New test for
	-mcase-values-threshold.
	* gcc.target/powerpc/ppc-switch-2.c: Ditto.

Comments

Richard Biener July 1, 2011, 7:57 a.m. UTC | #1
On Thu, Jun 30, 2011 at 7:06 PM, Michael Meissner
<meissner@linux.vnet.ibm.com> wrote:
> On Thu, Jun 30, 2011 at 12:31:44PM +0200, Richard Guenther wrote:
>> On Thu, Jun 30, 2011 at 12:34 AM, Michael Meissner
>> <meissner@linux.vnet.ibm.com> wrote:
>> > On the powerpc, switch statements can be expensive, and we would like to be
>> > able to tune the threshold of when the compiler generates if statements
>> > vs. using a table jump operation (and different processors within the powerpc
>> > have different limits).  This patch adds a powerpc tuning option to control
>> > this.
>> >
>> > I've done bootstraps and make checks with no regressions.  Is this ok to apply
>> > to the trunk?  At this time, I am not changing the default value (4).  With the
>> > option, I've seen a few spec 2006 benchmarks run faster, and a few run slower.
>>
>> Hmm.  This hook looks like it could be turned into a --param.  In fact
>> it doesn't get whatever context information, so I wonder if any target
>> does something fancy.
>
> I've done it also as a --param.  I tend to think it is better as a param, and
> then other people can tune their code.  One slight problem is the default for
> CASE_VALUES_THRESHOLD depends on whether you have a named casesi pattern that
> is active (HAVE_casesi ? 4 : 5).  Either we need two separate parameters, or we
> have just one parameter, and if that is 0, fall back to the 4 or 5 value.
>
> This patch swtiches to use --param to set the value.  Is it acceptable to check
> in?  I've done a bootstrap and I'm about to do a make check.

Looks good to me.  Please leave others 24h to comment on the
special zero value (no idea if we have precedent for it, but it sounds
ok to me).

Thanks,
Richard.

> [gcc]
> 2011-06-30  Michael Meissner  <meissner@linux.vnet.ibm.com>
>
>        * params.def (PARAM_CASE_VALUES_THRESHOLD): New parameter to
>        override CASE_VALUES_THRESHOLD.
>
>        * stmt.c (toplevel): Include params.h.
>        (case_values_threshold): Use the --param case-values-threshold
>        value if non-zero, otherwise use machine dependent value.
>        (expand_case): Use case_values_threshold.
>
>        * Makefile.in (stmt.o): Add $(PARAMS_H) dependency.
>
>        * doc/invoke.texi (--param case-values-threshold): Document.
>
> [gcc/testsuite]
> 2011-06-30  Michael Meissner  <meissner@linux.vnet.ibm.com>
>
>        * gcc.target/powerpc/ppc-switch-1.c: New test for
>        -mcase-values-threshold.
>        * gcc.target/powerpc/ppc-switch-2.c: Ditto.
>
> --
> Michael Meissner, IBM
> 5 Technology Place Drive, M/S 2757, Westford, MA 01886-3141, USA
> meissner@linux.vnet.ibm.com     fax +1 (978) 399-6899
>
Jakub Jelinek July 1, 2011, 8:01 a.m. UTC | #2
On Fri, Jul 01, 2011 at 09:57:41AM +0200, Richard Guenther wrote:
> Looks good to me.  Please leave others 24h to comment on the
> special zero value (no idea if we have precedent for it, but it sounds
> ok to me).

I think the special zero value is just fine, at least much nicer than the
http://gcc.gnu.org/ml/gcc-patches/2011-04/msg01807.html
version which would just confuse users.

	Jakub
diff mbox

Patch

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 175662)
+++ gcc/doc/invoke.texi	(working copy)
@@ -9026,6 +9026,11 @@  The maximum number of conditional stores
 if either vectorization (@option{-ftree-vectorize}) or if-conversion
 (@option{-ftree-loop-if-convert}) is disabled.  The default is 2.
 
+@item case-values-threshold
+The smallest number of different values for which it is best to use a
+jump-table instead of a tree of conditional branches.  If the value is
+0, use the default for the machine.  The default is 0.
+
 @end table
 @end table
 
Index: gcc/testsuite/gcc.target/powerpc/ppc-switch-1.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/ppc-switch-1.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/ppc-switch-1.c	(revision 0)
@@ -0,0 +1,26 @@ 
+/* { dg-do compile { target { powerpc*-*-* } } } */
+/* { dg-skip-if "" { powerpc*-*-darwin* } { "*" } { "" } } */
+/* { dg-options "-O2 --param case-values-threshold=2" } */
+/* { dg-final { scan-assembler "mtctr" } } */
+/* { dg-final { scan-assembler "bctr" } } */
+
+/* Force using a dispatch table even though by default we would generate
+   ifs.  */
+
+extern long call (long);
+
+long
+test_switch (long a, long b)
+{
+  long c;
+
+  switch (a)
+    {
+    case 0:  c = -b;	break;
+    case 1:  c = ~b;	break;
+    case 2:  c = b+1;	break;
+    default: c = b & 9;	break;
+    }
+
+  return call (c) + 1;
+}
Index: gcc/testsuite/gcc.target/powerpc/ppc-switch-2.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/ppc-switch-2.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/ppc-switch-2.c	(revision 0)
@@ -0,0 +1,32 @@ 
+/* { dg-do compile { target { powerpc*-*-* } } } */
+/* { dg-skip-if "" { powerpc*-*-darwin* } { "*" } { "" } } */
+/* { dg-options "-O2 --param case-values-threshold=20" } */
+/* { dg-final { scan-assembler-not "mtctr" } } */
+/* { dg-final { scan-assembler-not "bctr" } } */
+
+/* Force using if tests, instead of a dispatch table.  */
+
+extern long call (long);
+
+long
+test_switch (long a, long b)
+{
+  long c;
+
+  switch (a)
+    {
+    case 0:  c = -b;	break;
+    case 1:  c = ~b;	break;
+    case 2:  c = b+1;	break;
+    case 3:  c = b-2;	break;
+    case 4:  c = b*3;	break;
+    case 5:  c = b/4;	break;
+    case 6:  c = b<<5;	break;
+    case 7:  c = b>>6;	break;
+    case 8:  c = b|7;	break;
+    case 9:  c = b^8;	break;
+    default: c = b&9;	break;
+    }
+
+  return call (c) + 1;
+}
Index: gcc/Makefile.in
===================================================================
--- gcc/Makefile.in	(revision 175662)
+++ gcc/Makefile.in	(working copy)
@@ -2946,7 +2946,7 @@  stmt.o : stmt.c $(CONFIG_H) $(SYSTEM_H) 
    $(LIBFUNCS_H) $(EXCEPT_H) $(RECOG_H) $(DIAGNOSTIC_CORE_H) \
    output.h $(GGC_H) $(TM_P_H) langhooks.h $(PREDICT_H) $(OPTABS_H) \
    $(TARGET_H) $(GIMPLE_H) $(MACHMODE_H) $(REGS_H) alloc-pool.h \
-   $(PRETTY_PRINT_H) $(BITMAP_H)
+   $(PRETTY_PRINT_H) $(BITMAP_H) $(PARAMS_H)
 except.o : except.c $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TM_H) $(RTL_H) \
    $(TREE_H) $(FLAGS_H) $(EXCEPT_H) $(FUNCTION_H) $(EXPR_H) $(LIBFUNCS_H) \
    langhooks.h insn-config.h hard-reg-set.h $(BASIC_BLOCK_H) output.h \
Index: gcc/stmt.c
===================================================================
--- gcc/stmt.c	(revision 175662)
+++ gcc/stmt.c	(working copy)
@@ -53,6 +53,7 @@  along with GCC; see the file COPYING3.  
 #include "alloc-pool.h"
 #include "pretty-print.h"
 #include "bitmap.h"
+#include "params.h"
 
 
 /* Functions and data structures for expanding case statements.  */
@@ -2270,6 +2271,20 @@  expand_switch_using_bit_tests_p (tree in
 	      || (uniq == 3 && count >= 6)));
 }
 
+/* Return the smallest number of different values for which it is best to use a
+   jump-table instead of a tree of conditional branches.  */
+
+static unsigned int
+case_values_threshold (void)
+{
+  unsigned int threshold = PARAM_VALUE (PARAM_CASE_VALUES_THRESHOLD);
+
+  if (threshold == 0)
+    threshold = targetm.case_values_threshold ();
+
+  return threshold;
+}
+
 /* Terminate a case (Pascal/Ada) or switch (C) statement
    in which ORIG_INDEX is the expression to be tested.
    If ORIG_TYPE is not NULL, it is the original ORIG_INDEX
@@ -2424,7 +2439,7 @@  expand_case (gimple stmt)
 	 If the switch-index is a constant, do it this way
 	 because we can optimize it.  */
 
-      else if (count < targetm.case_values_threshold ()
+      else if (count < case_values_threshold ()
 	       || compare_tree_int (range,
 				    (optimize_insn_for_size_p () ? 3 : 10) * count) > 0
 	       /* RANGE may be signed, and really large ranges will show up
Index: gcc/params.def
===================================================================
--- gcc/params.def	(revision 175662)
+++ gcc/params.def	(working copy)
@@ -892,6 +892,16 @@  DEFPARAM (PARAM_MAX_STORES_TO_SINK,
           "Maximum number of conditional store pairs that can be sunk",
           2, 0, 0)
 
+/* Override CASE_VALUES_THRESHOLD of when to switch from doing switch
+   statements via if statements to using a table jump operation.  If the value
+   is 0, the default CASE_VALUES_THRESHOLD will be used.  */
+DEFPARAM (PARAM_CASE_VALUES_THRESHOLD,
+          "case-values-threshold",
+          "The smallest number of different values for which it is best to "
+	  "use a jump-table instead of a tree of conditional branches, "
+	  "if 0, use the default for the machine",
+          0, 0, 0)
+
 
 /*
 Local variables: