Re: [patch] Cleanup tree-switch-conversion a bit
On Tue, Apr 17, 2012 at 9:15 AM, Steven Bosscher stevenb@gmail.com wrote: My goal for GCC 4.8 is to do just that: Move switch expansion to GIMPLE and add value profiling for switch expressions. And the idea is to put all that code in tree-switch-conversion.c. But there are a few clean-ups I wish to do on that code before that. First, there is a global pass info structure that contains useful data for all forms of GIMPLE_SWITCH lowering. I've un-globalized that data with the attached patch. While there, I made the dump messages uniform. Bootstrapped and tested on powerpc-unknown-linux-gnu. OK? I think it caused: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53043 -- H.J.
Re: [patch] Cleanup tree-switch-conversion a bit
On Thu, Apr 19, 2012 at 4:55 PM, H.J. Lu hjl.to...@gmail.com wrote:
On Tue, Apr 17, 2012 at 9:15 AM, Steven Bosscher stevenb@gmail.com
wrote:
My goal for GCC 4.8 is to do just that: Move switch expansion to
GIMPLE and add value profiling for switch expressions.
And the idea is to put all that code in tree-switch-conversion.c. But
there are a few clean-ups I wish to do on that code before that.
First, there is a global pass info structure that contains useful data
for all forms of GIMPLE_SWITCH lowering. I've un-globalized that
data with the attached patch. While there, I made the dump messages
uniform.
Bootstrapped and tested on powerpc-unknown-linux-gnu. OK?
I think it caused:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53043
Yes.
* gcc.target/i386/pr45830.c: Update scan-tree-dump.
Index: gcc.target/i386/pr45830.c
===
--- gcc.target/i386/pr45830.c (revision 186596)
+++ gcc.target/i386/pr45830.c (working copy)
@@ -26,6 +26,6 @@ foo (int *a)
}
}
-/* { dg-final { scan-tree-dump Expanding as bit test is preferable
switchconv } } */
+/* { dg-final { scan-tree-dump expanding as bit test is preferable
switchconv } } */
/* { dg-final { scan-assembler-not CSWTCH } } */
/* { dg-final { cleanup-tree-dump switchconv } } */
Re: [patch] Cleanup tree-switch-conversion a bit
On Tue, Apr 17, 2012 at 6:15 PM, Steven Bosscher stevenb@gmail.com wrote: My goal for GCC 4.8 is to do just that: Move switch expansion to GIMPLE and add value profiling for switch expressions. And the idea is to put all that code in tree-switch-conversion.c. But there are a few clean-ups I wish to do on that code before that. First, there is a global pass info structure that contains useful data for all forms of GIMPLE_SWITCH lowering. I've un-globalized that data with the attached patch. While there, I made the dump messages uniform. Bootstrapped and tested on powerpc-unknown-linux-gnu. OK? Ok. Thanks, Richard. Ciao! Steven
Re: [patch] Cleanup tree-switch-conversion a bit
On Wed, 18 Apr 2012, Steven Bosscher wrote: Hello, This is another step towards moving GIMPLE_SWITCH expansion to an earlier point in the pipeline. With the attached patch, some of the logic from stmt.c:add_case_node() is moved to gimplify.c:gimplify_switch_expr(). This includes: * Code to drop case labels that are out of range for the switch index expression. (Actually, I suspect this code hasn't worked properly since gimplification was introduced, because the switch index expression can be promoted by language specific gimplification, so expand_case never actually sees the proper type with the current implementation in stmt.c.) * Code to fold_convert case label values to the right type. I've opted to go for folding to the original type of the SWITCH_EXPR, rather than to the post-gimplification switch index type. * Code to canonicalize CASE_LABEL's subnodes, CASE_LOW and CASE_HIGH. I've chosen to impose strict requirements that CASE_HIGH CASE_LOW if CASE_HIGH is non-zero. This is different from what add_case_node does, but I think it makes sense to go for the minimal representation here: The case labels in stmt.c never lived very long (only during expand) but GIMPLE_SWITCH statements stay around for much of the compilation process and can also be streamed out, etc. Bootstrapped and tested on powerpc-unknown-linux-gnu. OK for trunk? I wonder about @@ -1575,6 +1575,9 @@ gimplify_switch_expr (tree *expr_p, gimple_seq *pr tree switch_expr = *expr_p; gimple_seq switch_body_seq = NULL; enum gimplify_status ret; + tree index_type = TREE_TYPE (switch_expr); + if (index_type == void_type_node) +index_type = TREE_TYPE (SWITCH_COND (switch_expr)); what frontends use void_type_node here contrary to the docs in tree.def: TREE_TYPE is the original type of the condition, before any language required type conversions. It may be NULL, in which case the original type and final types are assumed to be the same. which says you'd need to handle NULL instead? Thus, can you either adjust the docs in tree.def or the frontends? Ok with either change. Thanks, Richard.
Re: [patch] Cleanup tree-switch-conversion a bit
On Wed, Apr 18, 2012 at 11:10 AM, Richard Guenther rguent...@suse.de wrote: I wonder about @@ -1575,6 +1575,9 @@ gimplify_switch_expr (tree *expr_p, gimple_seq *pr tree switch_expr = *expr_p; gimple_seq switch_body_seq = NULL; enum gimplify_status ret; + tree index_type = TREE_TYPE (switch_expr); + if (index_type == void_type_node) + index_type = TREE_TYPE (SWITCH_COND (switch_expr)); what frontends use void_type_node here contrary to the docs in tree.def: TREE_TYPE is the original type of the condition, before any language required type conversions. It may be NULL, in which case the original type and final types are assumed to be the same. which says you'd need to handle NULL instead? Thus, can you either adjust the docs in tree.def or the frontends? That code was copied from stmt.c, and I was surprised by it, too. The Fortran frond end builds SWITCH_EXPRs with build3_v, i.e. void_type_node-typed. The Go front ends also build SWITCH_EXPRs with void_type_node. The C-family and Java front ends build SWITCH_EXPRs with a non-void type. But from gimplify.c:is_gimple_stmt(): case SWITCH_EXPR: (...) /* These are always void. */ return true; See these files for all the locations I could find where a SWITCH_EXPR is built: java/expr.c: switch_expr = build3 (SWITCH_EXPR, TREE_TYPE (selector), selector, go/go-gcc.cc: tree t = build3_loc(switch_location.gcc_location(), SWITCH_EXPR, cp/cp-gimplify.c: t = build3 (SWITCH_EXPR, SWITCH_STMT_TYPE (stmt), c-typeck.c: cs-switch_expr = build3 (SWITCH_EXPR, orig_type, exp, NULL_TREE, NULL_TREE); fortran/trans-stmt.c: tmp = build3_v (SWITCH_EXPR, se.expr, tmp, NULL_TREE); fortran/trans-stmt.c: tmp = build3_v (SWITCH_EXPR, case_num, tmp, NULL_TREE); fortran/trans-stmt.c: tmp = build3_v (SWITCH_EXPR, case_num, tmp, NULL_TREE); fortran/trans-io.c: tmp = build3_v (SWITCH_EXPR, rc, tmp, NULL_TREE); fortran/trans-decl.c: tmp = build3_v (SWITCH_EXPR, val, tmp, NULL_TREE); ada/gcc-interface/trans.c: gnu_result = build3 (SWITCH_EXPR, TREE_TYPE (gnu_expr), gnu_expr, Perhaps the Fortran and Go front ends should build SWITCH_EXPRs with a NULL type, and gimplify_switch_expr() should assert that the SWITCH_EXPR type is never void_type node. That would make things match the documentation in tree.def again. I'll test a patch with these changes and commit it if it works. Ciao! Steven
Re: [patch] Cleanup tree-switch-conversion a bit
On Wed, 18 Apr 2012, Steven Bosscher wrote: On Wed, Apr 18, 2012 at 11:10 AM, Richard Guenther rguent...@suse.de wrote: I wonder about @@ -1575,6 +1575,9 @@ gimplify_switch_expr (tree *expr_p, gimple_seq *pr tree switch_expr = *expr_p; gimple_seq switch_body_seq = NULL; enum gimplify_status ret; + tree index_type = TREE_TYPE (switch_expr); + if (index_type == void_type_node) + index_type = TREE_TYPE (SWITCH_COND (switch_expr)); what frontends use void_type_node here contrary to the docs in tree.def: TREE_TYPE is the original type of the condition, before any language required type conversions. It may be NULL, in which case the original type and final types are assumed to be the same. which says you'd need to handle NULL instead? Thus, can you either adjust the docs in tree.def or the frontends? That code was copied from stmt.c, and I was surprised by it, too. The Fortran frond end builds SWITCH_EXPRs with build3_v, i.e. void_type_node-typed. The Go front ends also build SWITCH_EXPRs with void_type_node. The C-family and Java front ends build SWITCH_EXPRs with a non-void type. But from gimplify.c:is_gimple_stmt(): case SWITCH_EXPR: (...) /* These are always void. */ return true; See these files for all the locations I could find where a SWITCH_EXPR is built: java/expr.c: switch_expr = build3 (SWITCH_EXPR, TREE_TYPE (selector), selector, go/go-gcc.cc: tree t = build3_loc(switch_location.gcc_location(), SWITCH_EXPR, cp/cp-gimplify.c: t = build3 (SWITCH_EXPR, SWITCH_STMT_TYPE (stmt), c-typeck.c: cs-switch_expr = build3 (SWITCH_EXPR, orig_type, exp, NULL_TREE, NULL_TREE); fortran/trans-stmt.c: tmp = build3_v (SWITCH_EXPR, se.expr, tmp, NULL_TREE); fortran/trans-stmt.c: tmp = build3_v (SWITCH_EXPR, case_num, tmp, NULL_TREE); fortran/trans-stmt.c: tmp = build3_v (SWITCH_EXPR, case_num, tmp, NULL_TREE); fortran/trans-io.c: tmp = build3_v (SWITCH_EXPR, rc, tmp, NULL_TREE); fortran/trans-decl.c: tmp = build3_v (SWITCH_EXPR, val, tmp, NULL_TREE); ada/gcc-interface/trans.c: gnu_result = build3 (SWITCH_EXPR, TREE_TYPE (gnu_expr), gnu_expr, Perhaps the Fortran and Go front ends should build SWITCH_EXPRs with a NULL type, and gimplify_switch_expr() should assert that the SWITCH_EXPR type is never void_type node. That would make things match the documentation in tree.def again. I'll test a patch with these changes and commit it if it works. I suppose generic tree handling routines are confused by NULL TREE_TYPE and thus changing the docs to void_type_node would be more appropriate. Richard.
Re: [patch] Cleanup tree-switch-conversion a bit
On Wed, Apr 18, 2012 at 1:23 PM, Richard Guenther rguent...@suse.de wrote: I suppose generic tree handling routines are confused by NULL TREE_TYPE and thus changing the docs to void_type_node would be more appropriate. I don't agree with that. The documented behavior is much older than either the Fortran or the go front end, so the safest way is to fix those front ends to match the documentation. With my patch modified as attached, bootstraptest still passes. OK? Ciao! Steven fix_SWITCH_EXPR_builders.diff Description: Binary data
Re: [patch] Cleanup tree-switch-conversion a bit
On Wed, Apr 18, 2012 at 8:30 PM, Steven Bosscher stevenb@gmail.com wrote: The Go bits approved on IRC by Iant, the Fortran bits are obvious, and the rest was already approved. This is r186579 now. And because I managed to commit from the wrong tree, the fixed commit is r186580. Index: gimplify.c === --- gimplify.c (revision 186579) +++ gimplify.c (working copy) @@ -1578,7 +1578,6 @@ gimplify_switch_expr (tree *expr_p, gimple_seq *pr tree index_type = TREE_TYPE (switch_expr); if (index_type == NULL_TREE) index_type = TREE_TYPE (SWITCH_COND (switch_expr)); - gcc_assert (INTEGRAL_TYPE_P (index_type)); ret = gimplify_expr (SWITCH_COND (switch_expr), pre_p, NULL, is_gimple_val, fb_rvalue);
[patch] Cleanup tree-switch-conversion a bit
My goal for GCC 4.8 is to do just that: Move switch expansion to
GIMPLE and add value profiling for switch expressions.
And the idea is to put all that code in tree-switch-conversion.c. But
there are a few clean-ups I wish to do on that code before that.
First, there is a global pass info structure that contains useful data
for all forms of GIMPLE_SWITCH lowering. I've un-globalized that
data with the attached patch. While there, I made the dump messages
uniform.
Bootstrapped and tested on powerpc-unknown-linux-gnu. OK?
Ciao!
Steven
* tree-switch-conversion.c (info): Remove global pass info.
(check_range, check_process_case, check_final_bb, create_temp_arrays,
free_temp_arrays, gather_default_values, build_constructors,
array_value_type, build_one_array, build_arrays, gen_def_assigns,
fix_phi_nodes, gen_inbound_check): Pass info around from ...
(process_switch): ... here. Unify message format. Return a const
char pointer to the failure reason message.
(do_switchconv): Unify message format. Update process_switch usage.
Index: tree-switch-conversion.c
===
--- tree-switch-conversion.c(revision 186526)
+++ tree-switch-conversion.c(working copy)
@@ -24,8 +24,8 @@ Software Foundation, 51 Franklin Street, Fifth Flo
Switch initialization conversion
The following pass changes simple initializations of scalars in a switch
-statement into initializations from a static array. Obviously, the values must
-be constant and known at compile time and a default branch must be
+statement into initializations from a static array. Obviously, the values
+must be constant and known at compile time and a default branch must be
provided. For example, the following code:
int a,b;
@@ -162,16 +162,12 @@ struct switch_conv_info
basic_block bit_test_bb[2];
};
-/* Global pass info. */
-static struct switch_conv_info info;
-
-
/* Checks whether the range given by individual case statements of the SWTCH
switch statement isn't too big and whether the number of branches actually
satisfies the size of the new array. */
static bool
-check_range (gimple swtch)
+check_range (gimple swtch, struct switch_conv_info *info)
{
tree min_case, max_case;
unsigned int branch_num = gimple_switch_num_labels (swtch);
@@ -181,7 +177,7 @@ static bool
is a default label which is the first in the vector. */
min_case = gimple_switch_label (swtch, 1);
- info.range_min = CASE_LOW (min_case);
+ info-range_min = CASE_LOW (min_case);
gcc_assert (branch_num 1);
gcc_assert (CASE_LOW (gimple_switch_label (swtch, 0)) == NULL_TREE);
@@ -191,22 +187,22 @@ static bool
else
range_max = CASE_LOW (max_case);
- gcc_assert (info.range_min);
+ gcc_assert (info-range_min);
gcc_assert (range_max);
- info.range_size = int_const_binop (MINUS_EXPR, range_max, info.range_min);
+ info-range_size = int_const_binop (MINUS_EXPR, range_max, info-range_min);
- gcc_assert (info.range_size);
- if (!host_integerp (info.range_size, 1))
+ gcc_assert (info-range_size);
+ if (!host_integerp (info-range_size, 1))
{
- info.reason = index range way too large or otherwise unusable.\n;
+ info-reason = index range way too large or otherwise unusable;
return false;
}
- if ((unsigned HOST_WIDE_INT) tree_low_cst (info.range_size, 1)
+ if ((unsigned HOST_WIDE_INT) tree_low_cst (info-range_size, 1)
((unsigned) branch_num * SWITCH_CONVERSION_BRANCH_RATIO))
{
- info.reason = the maximum range-branch ratio exceeded.\n;
+ info-reason = the maximum range-branch ratio exceeded;
return false;
}
@@ -219,7 +215,7 @@ static bool
and returns true. Otherwise returns false. */
static bool
-check_process_case (tree cs)
+check_process_case (tree cs, struct switch_conv_info *info)
{
tree ldecl;
basic_block label_bb, following_bb;
@@ -228,48 +224,48 @@ static bool
ldecl = CASE_LABEL (cs);
label_bb = label_to_block (ldecl);
- e = find_edge (info.switch_bb, label_bb);
+ e = find_edge (info-switch_bb, label_bb);
gcc_assert (e);
if (CASE_LOW (cs) == NULL_TREE)
{
/* Default branch. */
- info.default_prob = e-probability;
- info.default_count = e-count;
+ info-default_prob = e-probability;
+ info-default_count = e-count;
}
else
{
int i;
- info.other_count += e-count;
+ info-other_count += e-count;
for (i = 0; i 2; i++)
- if (info.bit_test_bb[i] == label_bb)
+ if (info-bit_test_bb[i] == label_bb)
break;
- else if (info.bit_test_bb[i] == NULL)
+ else if (info-bit_test_bb[i] == NULL)
{
- info.bit_test_bb[i] = label_bb;
- info.bit_test_uniq++;
+ info-bit_test_bb[i] = label_bb;
+ info-bit_test_uniq++;
break;
}
if (i
[patch] Cleanup tree-switch-conversion a bit
Hello,
This is another step towards moving GIMPLE_SWITCH expansion to an
earlier point in the pipeline.
With the attached patch, some of the logic from stmt.c:add_case_node()
is moved to gimplify.c:gimplify_switch_expr(). This includes:
* Code to drop case labels that are out of range for the switch index
expression. (Actually, I suspect this code hasn't worked properly
since gimplification was introduced, because the switch index
expression can be promoted by language specific gimplification, so
expand_case never actually sees the proper type with the current
implementation in stmt.c.)
* Code to fold_convert case label values to the right type. I've opted
to go for folding to the original type of the SWITCH_EXPR, rather than
to the post-gimplification switch index type.
* Code to canonicalize CASE_LABEL's subnodes, CASE_LOW and CASE_HIGH.
I've chosen to impose strict requirements that CASE_HIGH CASE_LOW if
CASE_HIGH is non-zero. This is different from what add_case_node does,
but I think it makes sense to go for the minimal representation here:
The case labels in stmt.c never lived very long (only during expand)
but GIMPLE_SWITCH statements stay around for much of the compilation
process and can also be streamed out, etc.
Bootstrapped and tested on powerpc-unknown-linux-gnu. OK for trunk?
Ciao!
Steven
* targhooks.c (default_case_values_threshold): Fix code style nit.
* stmt.c (add_case_node, expand_case): Move logic to remove/reduce
case range and type folding from here...
* gimplify.c (gimplify_switch_expr): ... to here.
Index: targhooks.c
===
--- targhooks.c (revision 186526)
+++ targhooks.c (working copy)
@@ -1200,7 +1200,8 @@ default_target_can_inline_p (tree caller, tree cal
this means extra overhead for dispatch tables, which raises the
threshold for using them. */
-unsigned int default_case_values_threshold (void)
+unsigned int
+default_case_values_threshold (void)
{
return (HAVE_casesi ? 4 : 5);
}
Index: tree-switch-conversion.c
===
--- tree-switch-conversion.c(revision 186526)
+++ tree-switch-conversion.c(working copy)
@@ -24,8 +24,8 @@ Software Foundation, 51 Franklin Street, Fifth Flo
Switch initialization conversion
The following pass changes simple initializations of scalars in a switch
-statement into initializations from a static array. Obviously, the values must
-be constant and known at compile time and a default branch must be
+statement into initializations from a static array. Obviously, the values
+must be constant and known at compile time and a default branch must be
provided. For example, the following code:
int a,b;
@@ -162,16 +162,12 @@ struct switch_conv_info
basic_block bit_test_bb[2];
};
-/* Global pass info. */
-static struct switch_conv_info info;
-
-
/* Checks whether the range given by individual case statements of the SWTCH
switch statement isn't too big and whether the number of branches actually
satisfies the size of the new array. */
static bool
-check_range (gimple swtch)
+check_range (gimple swtch, struct switch_conv_info *info)
{
tree min_case, max_case;
unsigned int branch_num = gimple_switch_num_labels (swtch);
@@ -181,7 +177,7 @@ static bool
is a default label which is the first in the vector. */
min_case = gimple_switch_label (swtch, 1);
- info.range_min = CASE_LOW (min_case);
+ info-range_min = CASE_LOW (min_case);
gcc_assert (branch_num 1);
gcc_assert (CASE_LOW (gimple_switch_label (swtch, 0)) == NULL_TREE);
@@ -191,22 +187,22 @@ static bool
else
range_max = CASE_LOW (max_case);
- gcc_assert (info.range_min);
+ gcc_assert (info-range_min);
gcc_assert (range_max);
- info.range_size = int_const_binop (MINUS_EXPR, range_max, info.range_min);
+ info-range_size = int_const_binop (MINUS_EXPR, range_max, info-range_min);
- gcc_assert (info.range_size);
- if (!host_integerp (info.range_size, 1))
+ gcc_assert (info-range_size);
+ if (!host_integerp (info-range_size, 1))
{
- info.reason = index range way too large or otherwise unusable.\n;
+ info-reason = index range way too large or otherwise unusable;
return false;
}
- if ((unsigned HOST_WIDE_INT) tree_low_cst (info.range_size, 1)
+ if ((unsigned HOST_WIDE_INT) tree_low_cst (info-range_size, 1)
((unsigned) branch_num * SWITCH_CONVERSION_BRANCH_RATIO))
{
- info.reason = the maximum range-branch ratio exceeded.\n;
+ info-reason = the maximum range-branch ratio exceeded;
return false;
}
@@ -219,7 +215,7 @@ static bool
and returns true. Otherwise returns false. */
static bool
-check_process_case (tree cs)
+check_process_case (tree cs, struct switch_conv_info *info)
{
tree ldecl;
basic_block label_bb, following_bb;
@@ -228,48 +224,48 @@ static bool
