Re: [PATCH 1/4] Transform switch_conversion into a class.
On 06/20/2018 02:38 AM, Martin Liška wrote: > On 06/12/2018 09:41 PM, Jeff Law wrote: >> On 06/04/2018 07:32 AM, marxin wrote: >>> gcc/ChangeLog: >>> >>> 2018-06-07 Martin Liska >>> >>> * tree-switch-conversion.c (MAX_CASE_BIT_TESTS): Remove. >>> (hoist_edge_and_branch_if_true): Likewise. >>> (expand_switch_using_bit_tests_p): Likewise. >>> (struct case_bit_test): Likewise. >>> (case_bit_test_cmp): Likewise. >>> (emit_case_bit_tests): Likewise. >>> (switch_conversion::switch_conversion): New class. >>> (struct switch_conv_info): Remove old struct. >>> (collect_switch_conv_info): More to ... >>> (switch_conversion::collect): ... this. >>> (check_range): Likewise. >>> (switch_conversion::check_range): Likewise. >>> (check_all_empty_except_final): Likewise. >>> (switch_conversion::check_all_empty_except_final): Likewise. >>> (check_final_bb): Likewise. >>> (switch_conversion::check_final_bb): Likewise. >>> (create_temp_arrays): Likewise. >>> (switch_conversion::create_temp_arrays): Likewise. >>> (free_temp_arrays): Likewise. >>> (gather_default_values): Likewise. >>> (switch_conversion::gather_default_values): Likewise. >>> (build_constructors): Likewise. >>> (switch_conversion::build_constructors): Likewise. >>> (constructor_contains_same_values_p): Likewise. >>> (switch_conversion::contains_same_values_p): Likewise. >>> (array_value_type): Likewise. >>> (switch_conversion::array_value_type): Likewise. >>> (build_one_array): Likewise. >>> (switch_conversion::build_one_array): Likewise. >>> (build_arrays): Likewise. >>> (switch_conversion::build_arrays): Likewise. >>> (gen_def_assigns): Likewise. >>> (switch_conversion::gen_def_assigns): Likewise. >>> (prune_bbs): Likewise. >>> (switch_conversion::prune_bbs): Likewise. >>> (fix_phi_nodes): Likewise. >>> (switch_conversion::fix_phi_nodes): Likewise. >>> (gen_inbound_check): Likewise. >>> (switch_conversion::gen_inbound_check): Likewise. >>> (process_switch): Use the newly created class. >>> (switch_conversion::expand): New. >>> (switch_conversion::~switch_conversion): New. >>> * tree-switch-conversion.h: New file. >> So generally this looks good. >> >> I do note that a lot of the function comments are removed from the >> implementation side and moved into the header file. > > Hi. > > Thanks for review. > >> >> I personally prefer that approach, particularly for publicly visible >> members. However I think our coding conventions still indicate the >> implementation side should have the function comment. So let's keep >> those in place, with obvious changes since many of the arguments are now >> class data. > > I'm going to change it in order to follow our coding style. > Question is whether we can change that for future? Note that > there are quite long function comments in tree-switch-conversion.h. I'll support a change to the conventions. In a perfect world the .h file would provide an interface and the comments in there would be sufficient for anyone to use the interface. Any comments in the .c would cover implementation issues. I don't know if we'll get to that point, but that's the mental model in my head. jeff
Re: [PATCH 1/4] Transform switch_conversion into a class.
On 06/12/2018 09:41 PM, Jeff Law wrote: > On 06/04/2018 07:32 AM, marxin wrote: >> gcc/ChangeLog: >> >> 2018-06-07 Martin Liska >> >> * tree-switch-conversion.c (MAX_CASE_BIT_TESTS): Remove. >> (hoist_edge_and_branch_if_true): Likewise. >> (expand_switch_using_bit_tests_p): Likewise. >> (struct case_bit_test): Likewise. >> (case_bit_test_cmp): Likewise. >> (emit_case_bit_tests): Likewise. >> (switch_conversion::switch_conversion): New class. >> (struct switch_conv_info): Remove old struct. >> (collect_switch_conv_info): More to ... >> (switch_conversion::collect): ... this. >> (check_range): Likewise. >> (switch_conversion::check_range): Likewise. >> (check_all_empty_except_final): Likewise. >> (switch_conversion::check_all_empty_except_final): Likewise. >> (check_final_bb): Likewise. >> (switch_conversion::check_final_bb): Likewise. >> (create_temp_arrays): Likewise. >> (switch_conversion::create_temp_arrays): Likewise. >> (free_temp_arrays): Likewise. >> (gather_default_values): Likewise. >> (switch_conversion::gather_default_values): Likewise. >> (build_constructors): Likewise. >> (switch_conversion::build_constructors): Likewise. >> (constructor_contains_same_values_p): Likewise. >> (switch_conversion::contains_same_values_p): Likewise. >> (array_value_type): Likewise. >> (switch_conversion::array_value_type): Likewise. >> (build_one_array): Likewise. >> (switch_conversion::build_one_array): Likewise. >> (build_arrays): Likewise. >> (switch_conversion::build_arrays): Likewise. >> (gen_def_assigns): Likewise. >> (switch_conversion::gen_def_assigns): Likewise. >> (prune_bbs): Likewise. >> (switch_conversion::prune_bbs): Likewise. >> (fix_phi_nodes): Likewise. >> (switch_conversion::fix_phi_nodes): Likewise. >> (gen_inbound_check): Likewise. >> (switch_conversion::gen_inbound_check): Likewise. >> (process_switch): Use the newly created class. >> (switch_conversion::expand): New. >> (switch_conversion::~switch_conversion): New. >> * tree-switch-conversion.h: New file. > So generally this looks good. > > I do note that a lot of the function comments are removed from the > implementation side and moved into the header file. Hi. Thanks for review. > > I personally prefer that approach, particularly for publicly visible > members. However I think our coding conventions still indicate the > implementation side should have the function comment. So let's keep > those in place, with obvious changes since many of the arguments are now > class data. I'm going to change it in order to follow our coding style. Question is whether we can change that for future? Note that there are quite long function comments in tree-switch-conversion.h. Martin > > During my initial high level pass over the changes it looked like > various chunks of functionality were missing. It turns out they were > removed in patch #1, then re-introduced in subsequent patches. That's > fine, but it's slightly better from a review standpoint if such things > are at least called out. > > >> +using namespace tree_switch_conversion; > I thought our coding conventions discouraged "using namespace". But > after reviewing, that's for header files, so I won't object to this. > > > > This patch is fine. However, please do not install until the full set > is approved. > > Thanks, > jeff >
Re: [PATCH 1/4] Transform switch_conversion into a class.
On 06/04/2018 07:32 AM, marxin wrote: > gcc/ChangeLog: > > 2018-06-07 Martin Liska > > * tree-switch-conversion.c (MAX_CASE_BIT_TESTS): Remove. > (hoist_edge_and_branch_if_true): Likewise. > (expand_switch_using_bit_tests_p): Likewise. > (struct case_bit_test): Likewise. > (case_bit_test_cmp): Likewise. > (emit_case_bit_tests): Likewise. > (switch_conversion::switch_conversion): New class. > (struct switch_conv_info): Remove old struct. > (collect_switch_conv_info): More to ... > (switch_conversion::collect): ... this. > (check_range): Likewise. > (switch_conversion::check_range): Likewise. > (check_all_empty_except_final): Likewise. > (switch_conversion::check_all_empty_except_final): Likewise. > (check_final_bb): Likewise. > (switch_conversion::check_final_bb): Likewise. > (create_temp_arrays): Likewise. > (switch_conversion::create_temp_arrays): Likewise. > (free_temp_arrays): Likewise. > (gather_default_values): Likewise. > (switch_conversion::gather_default_values): Likewise. > (build_constructors): Likewise. > (switch_conversion::build_constructors): Likewise. > (constructor_contains_same_values_p): Likewise. > (switch_conversion::contains_same_values_p): Likewise. > (array_value_type): Likewise. > (switch_conversion::array_value_type): Likewise. > (build_one_array): Likewise. > (switch_conversion::build_one_array): Likewise. > (build_arrays): Likewise. > (switch_conversion::build_arrays): Likewise. > (gen_def_assigns): Likewise. > (switch_conversion::gen_def_assigns): Likewise. > (prune_bbs): Likewise. > (switch_conversion::prune_bbs): Likewise. > (fix_phi_nodes): Likewise. > (switch_conversion::fix_phi_nodes): Likewise. > (gen_inbound_check): Likewise. > (switch_conversion::gen_inbound_check): Likewise. > (process_switch): Use the newly created class. > (switch_conversion::expand): New. > (switch_conversion::~switch_conversion): New. > * tree-switch-conversion.h: New file. So generally this looks good. I do note that a lot of the function comments are removed from the implementation side and moved into the header file. I personally prefer that approach, particularly for publicly visible members. However I think our coding conventions still indicate the implementation side should have the function comment. So let's keep those in place, with obvious changes since many of the arguments are now class data. During my initial high level pass over the changes it looked like various chunks of functionality were missing. It turns out they were removed in patch #1, then re-introduced in subsequent patches. That's fine, but it's slightly better from a review standpoint if such things are at least called out. > +using namespace tree_switch_conversion; I thought our coding conventions discouraged "using namespace". But after reviewing, that's for header files, so I won't object to this. This patch is fine. However, please do not install until the full set is approved. Thanks, jeff
[PATCH 1/4] Transform switch_conversion into a class.
gcc/ChangeLog:
2018-06-07 Martin Liska
* tree-switch-conversion.c (MAX_CASE_BIT_TESTS): Remove.
(hoist_edge_and_branch_if_true): Likewise.
(expand_switch_using_bit_tests_p): Likewise.
(struct case_bit_test): Likewise.
(case_bit_test_cmp): Likewise.
(emit_case_bit_tests): Likewise.
(switch_conversion::switch_conversion): New class.
(struct switch_conv_info): Remove old struct.
(collect_switch_conv_info): More to ...
(switch_conversion::collect): ... this.
(check_range): Likewise.
(switch_conversion::check_range): Likewise.
(check_all_empty_except_final): Likewise.
(switch_conversion::check_all_empty_except_final): Likewise.
(check_final_bb): Likewise.
(switch_conversion::check_final_bb): Likewise.
(create_temp_arrays): Likewise.
(switch_conversion::create_temp_arrays): Likewise.
(free_temp_arrays): Likewise.
(gather_default_values): Likewise.
(switch_conversion::gather_default_values): Likewise.
(build_constructors): Likewise.
(switch_conversion::build_constructors): Likewise.
(constructor_contains_same_values_p): Likewise.
(switch_conversion::contains_same_values_p): Likewise.
(array_value_type): Likewise.
(switch_conversion::array_value_type): Likewise.
(build_one_array): Likewise.
(switch_conversion::build_one_array): Likewise.
(build_arrays): Likewise.
(switch_conversion::build_arrays): Likewise.
(gen_def_assigns): Likewise.
(switch_conversion::gen_def_assigns): Likewise.
(prune_bbs): Likewise.
(switch_conversion::prune_bbs): Likewise.
(fix_phi_nodes): Likewise.
(switch_conversion::fix_phi_nodes): Likewise.
(gen_inbound_check): Likewise.
(switch_conversion::gen_inbound_check): Likewise.
(process_switch): Use the newly created class.
(switch_conversion::expand): New.
(switch_conversion::~switch_conversion): New.
* tree-switch-conversion.h: New file.
---
gcc/tree-switch-conversion.c | 1125 +++---
gcc/tree-switch-conversion.h | 283 +
2 files changed, 511 insertions(+), 897 deletions(-)
create mode 100644 gcc/tree-switch-conversion.h
diff --git a/gcc/tree-switch-conversion.c b/gcc/tree-switch-conversion.c
index dc60b34f506..2f848fcb6aa 100644
--- a/gcc/tree-switch-conversion.c
+++ b/gcc/tree-switch-conversion.c
@@ -55,626 +55,74 @@ Software Foundation, 51 Franklin Street, Fifth Floor, Boston, MA
type in the GIMPLE type system that is language-independent? */
#include "langhooks.h"
+#include "tree-switch-conversion.h"
-/* Maximum number of case bit tests.
- FIXME: This should be derived from PARAM_CASE_VALUES_THRESHOLD and
- targetm.case_values_threshold(), or be its own param. */
-#define MAX_CASE_BIT_TESTS 3
-
-/* Track whether or not we have altered the CFG and thus may need to
- cleanup the CFG when complete. */
-bool cfg_altered;
-
-/* Split the basic block at the statement pointed to by GSIP, and insert
- a branch to the target basic block of E_TRUE conditional on tree
- expression COND.
-
- It is assumed that there is already an edge from the to-be-split
- basic block to E_TRUE->dest block. This edge is removed, and the
- profile information on the edge is re-used for the new conditional
- jump.
-
- The CFG is updated. The dominator tree will not be valid after
- this transformation, but the immediate dominators are updated if
- UPDATE_DOMINATORS is true.
-
- Returns the newly created basic block. */
+using namespace tree_switch_conversion;
-static basic_block
-hoist_edge_and_branch_if_true (gimple_stmt_iterator *gsip,
- tree cond, edge e_true,
- bool update_dominators)
-{
- tree tmp;
- gcond *cond_stmt;
- edge e_false;
- basic_block new_bb, split_bb = gsi_bb (*gsip);
- bool dominated_e_true = false;
-
- gcc_assert (e_true->src == split_bb);
-
- if (update_dominators
- && get_immediate_dominator (CDI_DOMINATORS, e_true->dest) == split_bb)
-dominated_e_true = true;
-
- tmp = force_gimple_operand_gsi (gsip, cond, /*simple=*/true, NULL,
- /*before=*/true, GSI_SAME_STMT);
- cond_stmt = gimple_build_cond_from_tree (tmp, NULL_TREE, NULL_TREE);
- gsi_insert_before (gsip, cond_stmt, GSI_SAME_STMT);
-
- e_false = split_block (split_bb, cond_stmt);
- new_bb = e_false->dest;
- redirect_edge_pred (e_true, split_bb);
-
- e_true->flags &= ~EDGE_FALLTHRU;
- e_true->flags |= EDGE_TRUE_VALUE;
-
- e_false->flags &= ~EDGE_FALLTHRU;
- e_false->flags |= EDGE_FALSE_VALUE;
- e_false->probability = e_true->probability.invert ();
- new_bb->count = e_false->count ();
-
- if (update_dominators)
-{
- if (dominated_e_true)
- set_immediate_dominator (CDI_DOMINATORS, e_true->dest, split_bb);
- set_immediate_dominator (CDI_DOMINATORS
