Re: [PATCH 1/3] tree-ssa-tail-merge: add IPA ICF infrastructure.

[Jeff Law <law at redhat dot com>]

On 08/05/2015 09:16 AM, Martin Liška wrote:
> 2015-07-09  Martin Liska<mliska@suse.cz>
>
> 	* dbgcnt.def: Add new debug counter.
> 	* ipa-icf-gimple.c (func_checker::compare_ssa_name): Use newly added
> 	state flag.
> 	(func_checker::compare_memory_operand): Likewise.
> 	(func_checker::compare_cst_or_decl): Handle if we are in
> 	tail_merge_mode.
> 	(func_checker::reset_preferences): New function.
> 	(func_checker::set_comparing_sensitive_rhs): Likewise.
> 	(func_checker::stmt_local_def): New function.
> 	(func_checker::compare_phi_node): Move from sem_function class.
> 	(func_checker::compare_bb_tail_merge): New function.
> 	(func_checker::compare_bb): Improve STMT iteration.
> 	(func_checker::compare_gimple_call): Return false in case of
> 	an UBSAN function.
> 	(func_checker::compare_gimple_assign): Likewise.
> 	(func_checker::compare_gimple_label): Remove unused flag.
> 	(ssa_names_set): New class.
> 	(ssa_names_set::build): New function.
> 	* ipa-icf-gimple.h (func_checker::gsi_next_nonlocal): New
> 	function.
> 	(ssa_names_set::contains): New function.
> 	(ssa_names_set::add): Likewise.
> 	* ipa-icf.c (sem_function::equals_private): Use transformed
> 	function.
> 	(sem_function::compare_phi_node): Move to func_checker class.
> 	(make_pass_ipa_icf): Change namespace.
> 	* ipa-icf.h: Add new declarations and rename namespace.
> 	* tree-ssa-tail-merge.c (check_edges_correspondence): New
> 	function.
> 	(find_duplicate): Add usage of IPA ICF gimple infrastructure.
> 	(find_clusters_1): Pass new sem_function argument.
> 	(find_clusters): Likewise.
> 	(tail_merge_optimize): Call IPA ICF comparison machinery.
> 	(gvn_uses_equal): Remove.
> 	(gimple_equal_p): Likewise.
> 	(gsi_advance_bw_nondebug_nonlocal): Likewise.
> 	(find_duplicate): Remove unused argument.
> 	(make_pass_tail_merge): New function.
> 	(pass_tail_merge::execute): Likewise.
> 	(equal_ssa_uses): New function.
> 	(same_succ_hash): Skip hashing of call arguments.
> 	(same_succ_hash): Handle NULL value which can occur.
> 	(gimple_operand_equal_value_p): Remove.
> 	(same_phi_alternatives): Use newly added function equal_ssa_uses.
> 	(same_phi_alternatives_1): Pass a new argument.
> 	* passes.def: Add new pass.
> 	* tree-pass.h: Likewise.
> 	* tree-ssa-pre.c (pass_pre::execute): Remove connection to tail-merge
> 	pass.
> ---

> @@ -256,7 +265,8 @@ func_checker::compatible_types_p (tree t1, tree t2)
>     return true;
>   }
>
> -/* Function compare for equality given memory operands T1 and T2.  */
> +/* Function compare for equality given memory operands T1 and T2.
> +   If STRICT flag is true, versions must match strictly.  */
You've removed the STRICT argument, so you can probably drop this comment.

> @@ -626,6 +665,138 @@ func_checker::parse_labels (sem_bb *bb)
>       }
>   }
>
> +/* Return true if gimple STMT is just a local definition in a
> +   basic block.  Local definition in this context means that a product
> +   of the statement (transitively) does not escape the basic block.
> +   Used SSA names are contained in SSA_NAMES_SET.  */
> +
> +bool
> +func_checker::stmt_local_def (gimple stmt, ssa_names_set *ssa_names_set)
Funny, Kyrill just implemented something similar, but at the RTL level.


> @@ -1037,4 +1252,67 @@ func_checker::compare_gimple_asm (const gasm *g1, const gasm *g2)
>     return true;
>   }
>
> -} // ipa_icf_gimple namespace
> +void
> +ssa_names_set::build (basic_block bb)
My only concern here is whether or not the two passes are sufficient.  I 
can kind of intuitively see how it works most of the time, but what if 
BB is a single node loop (ie, it branches back to itself).  Do really 
get the transitive closure we want in that case?


So I think if you can assure me we're doing the right thing for single 
node loops in ssa_names_set::build and remove the one comment change 
noted above and we'll be good to go for the trunk.

jeff