Re: [PATCH 2/4] Switch other switch expansion methods into classes.

[Jeff Law <law at redhat dot com>]

On 06/05/2018 01:15 AM, marxin wrote:
> gcc/ChangeLog:
> 
> 2018-06-07  Martin Liska  <mliska@suse.cz>
> 
> 	* tree-switch-conversion.c (switch_conversion::collect):
>         Record m_uniq property.
> 	(switch_conversion::expand): Bail out for special conditions.
> 	(group_cluster::~group_cluster): New.
> 	(group_cluster::group_cluster): Likewise.
> 	(group_cluster::dump): Likewise.
> 	(jump_table_cluster::emit): New.
> 	(switch_decision_tree::fix_phi_operands_for_edges): New.
> 	(struct case_node): Remove struct.
> 	(jump_table_cluster::can_be_handled): New.
> 	(case_values_threshold): Moved to header.
> 	(reset_out_edges_aux): Likewise.
> 	(jump_table_cluster::is_beneficial): New.
> 	(bit_test_cluster::can_be_handled): Likewise.
> 	(add_case_node): Remove.
> 	(bit_test_cluster::is_beneficial): New.
> 	(case_bit_test::cmp): New.
> 	(bit_test_cluster::emit): New.
> 	(expand_switch_as_decision_tree_p): Remove.
> 	(bit_test_cluster::hoist_edge_and_branch_if_true): New.
> 	(fix_phi_operands_for_edge): Likewise.
> 	(switch_decision_tree::analyze_switch_statement): New.
> 	(compute_cases_per_edge): Move ...
> 	(switch_decision_tree::compute_cases_per_edge): ... here.
> 	(try_switch_expansion): Likewise.
> 	(switch_decision_tree::try_switch_expansion): Likewise.
> 	(record_phi_operand_mapping): Likewise.
> 	(switch_decision_tree::record_phi_operand_mapping): Likewise.
> 	(emit_case_decision_tree): Likewise.
> 	(switch_decision_tree::emit): Likewise.
> 	(balance_case_nodes): Likewise.
> 	(switch_decision_tree::balance_case_nodes): Likewise.
> 	(dump_case_nodes): Likewise.
> 	(switch_decision_tree::dump_case_nodes): Likewise.
> 	(emit_jump): Likewise.
> 	(switch_decision_tree::emit_jump): Likewise.
> 	(emit_cmp_and_jump_insns): Likewise.
> 	(switch_decision_tree::emit_cmp_and_jump_insns): Likewise.
> 	(emit_case_nodes): Likewise.
> 	(switch_decision_tree::emit_case_nodes): Likewise.
> 	(conditional_probability): Remove.
> 	* tree-switch-conversion.h (enum cluster_type): New.
> 	(PRINT_CASE): New.
> 	(struct cluster): Likewise.
> 	(cluster::cluster): Likewise.
> 	(struct simple_cluster): Likewise.
> 	(simple_cluster::simple_cluster): Likewise.
> 	(struct group_cluster): Likewise.
> 	(struct jump_table_cluster): Likewise.
> 	(struct bit_test_cluster): Likewise.
> 	(struct min_cluster_item): Likewise.
> 	(struct case_tree_node): Likewise.
> 	(case_tree_node::case_tree_node): Likewise.
> 	(jump_table_cluster::case_values_threshold): Likewise.
> 	(struct case_bit_test): Likewise.
> 	(struct switch_decision_tree): Likewise.
> 	(struct switch_conversion): Likewise.
> 	(switch_decision_tree::reset_out_edges_aux): Likewise.
> 
> gcc/testsuite/ChangeLog:
> 
> 2018-06-07  Martin Liska  <mliska@suse.cz>
> 
> 	* gcc.dg/tree-ssa/vrp104.c: Grep just for GIMPLE IL.
So like the previous patch, we need to make sure functions have their
function comments.


> ---
>  gcc/testsuite/gcc.dg/tree-ssa/vrp104.c |    2 +-
>  gcc/tree-switch-conversion.c           | 1343 ++++++++++++++----------
>  gcc/tree-switch-conversion.h           |  545 ++++++++++
>  3 files changed, 1325 insertions(+), 565 deletions(-)
> 
> 
> 0002-Switch-other-switch-expansion-methods-into-classes.patch
> 
> 
> +     The definition of "much bigger" depends on whether we are
> +     optimizing for size or for speed.  If the former, the maximum
> +     ratio range/count = 3, because this was found to be the optimal
> +     ratio for size on i686-pc-linux-gnu, see PR11823.  The ratio
> +     10 is much older, and was probably selected after an extensive
> +     benchmarking investigation on numerous platforms.  Or maybe it
> +     just made sense to someone at some point in the history of GCC,
> +     who knows...  */
"much older" is an understatement.  I believe the magic "10" pre-dates
my involvement in GCC.  You can find evidence of it as far back as
gcc-0.9.  I doubt it was extensively benchmarked, and even if it was,
the targets on which it was benchmarked don't reflect modern target
reality in terms of importance.

In general this patch is much harder to de-cipher as there's little
relation between chunks of code in the unidiff.  Given your long history
with GCC, I'm going to extend a lot of leeway that you got all this
stuff right as you moved code around.  However, in the future for this
kind of change we should seriously look at a different way to break down
a patch into meaningful chunks.

So, OK with adding the missing function comments.   I think that covers
the entire set.

Thanks,
Jeff