| Message ID | 20110421190210.GA12527@hungry-tiger.westford.ibm.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Apr 21, 2011 at 03:02:10PM -0400, Michael Meissner wrote: > In looking at some improvements to the powerpc, we wanted to change the default > for when a table jump is generated vs. a series of if statements. Now, we > could just add a powerpc specific TARGET_CASE_VALUES_THRESHOLD, but I tend to > think that these should be settable on all/most ports with --param. > > At present, there are only two ports (avr and mn10300) that define their own > TARGET_CASE_VALUES_THRESHOLD hook. My first patch does not remove the target > hook and modify the avr/mn10300 ports to use maybe_set_param_value, but that > can be done if desired. > > The patch adds two --param values, one for when the port is using the casesi > insn, and the other when it uses the more primitive tablejump insn. > > I have bootstrapped the compiler with this patch and run the test suite with no > regressions. Is it ok to apply as is? Should I modify the avr and mn10300 > ports to use the parameters and do away with the target hook? Or should I do > this just as a powerpc target hook? I never got a response for this, and my earlier ping didn't seem to go out. I'll check it in on Monday if there are no objections.
On Fri, May 06, 2011 at 12:21:24PM -0400, Michael Meissner wrote: > On Thu, Apr 21, 2011 at 03:02:10PM -0400, Michael Meissner wrote: > > In looking at some improvements to the powerpc, we wanted to change the default > > for when a table jump is generated vs. a series of if statements. Now, we > > could just add a powerpc specific TARGET_CASE_VALUES_THRESHOLD, but I tend to > > think that these should be settable on all/most ports with --param. > > > > At present, there are only two ports (avr and mn10300) that define their own > > TARGET_CASE_VALUES_THRESHOLD hook. My first patch does not remove the target > > hook and modify the avr/mn10300 ports to use maybe_set_param_value, but that > > can be done if desired. > > > > The patch adds two --param values, one for when the port is using the casesi > > insn, and the other when it uses the more primitive tablejump insn. > > > > I have bootstrapped the compiler with this patch and run the test suite with no > > regressions. Is it ok to apply as is? Should I modify the avr and mn10300 > > ports to use the parameters and do away with the target hook? Or should I do > > this just as a powerpc target hook? > > I never got a response for this, and my earlier ping didn't seem to go out. > I'll check it in on Monday if there are no objections. I think it is very weird to have two different params, if we need any such param, there should be just one and its default value should depend on HAVE_casesi. Jakub
On Fri, May 06, 2011 at 06:30:07PM +0200, Jakub Jelinek wrote: > On Fri, May 06, 2011 at 12:21:24PM -0400, Michael Meissner wrote: > > On Thu, Apr 21, 2011 at 03:02:10PM -0400, Michael Meissner wrote: > > > In looking at some improvements to the powerpc, we wanted to change the default > > > for when a table jump is generated vs. a series of if statements. Now, we > > > could just add a powerpc specific TARGET_CASE_VALUES_THRESHOLD, but I tend to > > > think that these should be settable on all/most ports with --param. > > > > > > At present, there are only two ports (avr and mn10300) that define their own > > > TARGET_CASE_VALUES_THRESHOLD hook. My first patch does not remove the target > > > hook and modify the avr/mn10300 ports to use maybe_set_param_value, but that > > > can be done if desired. > > > > > > The patch adds two --param values, one for when the port is using the casesi > > > insn, and the other when it uses the more primitive tablejump insn. > > > > > > I have bootstrapped the compiler with this patch and run the test suite with no > > > regressions. Is it ok to apply as is? Should I modify the avr and mn10300 > > > ports to use the parameters and do away with the target hook? Or should I do > > > this just as a powerpc target hook? > > > > I never got a response for this, and my earlier ping didn't seem to go out. > > I'll check it in on Monday if there are no objections. > > I think it is very weird to have two different params, if we need any such > param, there should be just one and its default value should depend on > HAVE_casesi. The problem is the values in params.def must be constant, and can't depend on switches. I imagine we can have a single param that is normally 0, and if it is non-zero use that value, otherwise fall back to (HAVE_casesi ? 4 : 5). Or we could set it in finish_options in opts.c. Any preference?
Index: gcc/params.def =================================================================== --- gcc/params.def (revision 172832) +++ gcc/params.def (working copy) @@ -885,6 +885,22 @@ DEFPARAM (PARAM_MAX_STORES_TO_SINK, 2, 0, 0) +/* Threshold for using jump table vs. if statements if the machine supports a + CASESI instruction. */ +DEFPARAM (PARAM_CASE_VALUES_THRESHOLD_CASESI, + "case-values-threshold-casesi", + "Minimum number of case elements to use a jump table instead of" + "if statements if the machine supports a CASESI instruction", + 4, 2, 0) + +/* Threshold for using jump table vs. if statements if the machine does not + support a CASESI instruction. */ +DEFPARAM (PARAM_CASE_VALUES_THRESHOLD_TABLEJUMP, + "case-values-threshold-tablejump", + "Minimum number of case elements to use a jump table instead of" + "if statements if the machine does not support a CASESI instruction", + 5, 2, 0) + /* Local variables: mode:c Index: gcc/params.h =================================================================== --- gcc/params.h (revision 172832) +++ gcc/params.h (working copy) @@ -206,4 +206,8 @@ extern void init_param_values (int *para PARAM_VALUE (PARAM_MIN_NONDEBUG_INSN_UID) #define MAX_STORES_TO_SINK \ PARAM_VALUE (PARAM_MAX_STORES_TO_SINK) +#define CASE_VALUES_THRESHOLD_CASESI \ + PARAM_VALUE (PARAM_CASE_VALUES_THRESHOLD_CASESI) +#define CASE_VALUES_THRESHOLD_TABLEJUMP \ + PARAM_VALUE (PARAM_CASE_VALUES_THRESHOLD_TABLEJUMP) #endif /* ! GCC_PARAMS_H */ Index: gcc/targhooks.c =================================================================== --- gcc/targhooks.c (revision 172832) +++ gcc/targhooks.c (working copy) @@ -71,7 +71,7 @@ along with GCC; see the file COPYING3. #include "opts.h" #include "tree-flow.h" #include "tree-ssa-alias.h" - +#include "params.h" bool default_legitimate_address_p (enum machine_mode mode ATTRIBUTE_UNUSED, @@ -1209,7 +1209,9 @@ default_target_can_inline_p (tree caller unsigned int default_case_values_threshold (void) { - return (HAVE_casesi ? 4 : 5); + return (HAVE_casesi + ? CASE_VALUES_THRESHOLD_CASESI + : CASE_VALUES_THRESHOLD_TABLEJUMP); } bool Index: gcc/Makefile.in =================================================================== --- gcc/Makefile.in (revision 172832) +++ gcc/Makefile.in (working copy) @@ -2807,7 +2807,7 @@ opts-common.o : opts-common.c $(OPTS_H) targhooks.o : targhooks.c $(CONFIG_H) $(SYSTEM_H) coretypes.h $(TREE_H) \ $(EXPR_H) $(TM_H) $(RTL_H) $(TM_P_H) $(FUNCTION_H) output.h $(DIAGNOSTIC_CORE_H) \ $(MACHMODE_H) $(TARGET_DEF_H) $(TARGET_H) $(GGC_H) gt-targhooks.h \ - $(OPTABS_H) $(RECOG_H) reload.h hard-reg-set.h intl.h $(OPTS_H) \ + $(OPTABS_H) $(RECOG_H) reload.h hard-reg-set.h intl.h $(OPTS_H) $(PARAMS_H) \ tree-ssa-alias.h $(TREE_FLOW_H) bversion.h: s-bversion; @true Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 172832) +++ gcc/doc/invoke.texi (working copy) @@ -8889,6 +8889,16 @@ 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-casesi +The minimum number of case elements to use a jump table instead of if +statements if the machine supports a @code{casesi} instruction. The +default is 4. + +@item case-values-threshold-tablejump +The minimum number of case elements to use a jump table instead of if +statements if the machine does not support a @code{casesi} +instruction. The default is 5. + @end table @end table Index: gcc/testsuite/gcc.target/powerpc/case-threshold1.c =================================================================== --- gcc/testsuite/gcc.target/powerpc/case-threshold1.c (revision 0) +++ gcc/testsuite/gcc.target/powerpc/case-threshold1.c (revision 0) @@ -0,0 +1,26 @@ +/* Insure case-values-threshold is obeyed. This test should generate if + statements and not a table jump. */ +/* { dg-do compile } */ +/* { dg-options "-O2 --param case-values-threshold-tablejump=11" } */ + +/* { dg-final { scan-assembler-not "mtctr" } } */ +/* { dg-final { scan-assembler-not "bctr" } } */ + +int +switch_10 (int a, int b) +{ + switch (a) + { + case 0: return b + 1; + case 1: return b - 2; + case 2: return b * 3; + case 3: return b / 4; + case 4: return b % 5; + case 5: return b << 6; + case 6: return b >> 7; + case 7: return b & 0x8; + case 8: return b | 0x9; + case 9: return b ^ 0xa; + default: return b; + } +} Index: gcc/testsuite/gcc.target/powerpc/case-threshold2.c =================================================================== --- gcc/testsuite/gcc.target/powerpc/case-threshold2.c (revision 0) +++ gcc/testsuite/gcc.target/powerpc/case-threshold2.c (revision 0) @@ -0,0 +1,26 @@ +/* Insure case-values-threshold is obeyed. This test should generate a table + jump and not if statements. */ +/* { dg-do compile } */ +/* { dg-options "-O2 --param case-values-threshold-tablejump=10" } */ + +/* { dg-final { scan-assembler "mtctr" } } */ +/* { dg-final { scan-assembler "bctr" } } */ + +int +switch_10 (int a, int b) +{ + switch (a) + { + case 0: return b + 1; + case 1: return b - 2; + case 2: return b * 3; + case 3: return b / 4; + case 4: return b % 5; + case 5: return b << 6; + case 6: return b >> 7; + case 7: return b & 0x8; + case 8: return b | 0x9; + case 9: return b ^ 0xa; + default: return b; + } +}