Re: [PATCH 1/3] tree-ssa-tail-merge: add IPA ICF infrastructure.
On 08/05/2015 09:16 AM, Martin Liška wrote: 2015-07-09 Martin Liskamli...@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
Re: [PATCH 1/3] tree-ssa-tail-merge: add IPA ICF infrastructure.
On 08/03/2015 07:38 PM, Jeff Law wrote: On 07/16/2015 05:03 AM, Martin Liška wrote: So a general question. We're passing in STRICT to several routines, which is fine. But then we're also checking M_TAIL_MERGE_MODE. What's the difference between the two? Can they be unified? Hello. I would say that STRICT is a bit generic mechanism that was introduced some time before. It's e.g. used for checking of THIS arguments for methods and make checking more sensitive in situations that are somehow special. The newly added state is orthogonal to the previous one. Fair enough. There's some cases where we've documented STRICT, and others where we haven't. If STRICT flag is true, version must match strictly Appears as documentation for STRICT. It seems like it'd be better to describe what strictly means here. Elsewhere we have comments like: Be strict in case of tail-merge optimization Which tends to confuse things a bit. Perhaps something more like: In the case of tail merge optimization, XYZ must match It seems like a nit, but to someone else reading the code I don't think the distinctions are all that clear right now, so if we can improve things, it'd be good. Hello Jeff. I decided to come up a bit different approach (I hope it's much saner). Instead of passing a new argument (called strict), I have rewritten func_checker class to work with a new inner state (m_comparing_sensitive_rhs), which is turned on in cases where we compare a RHS operand. Usage of the flag is placed to the corresponding part of func_checker. I didn't find this comment particularly useful in understanding what this function does. AFAICT the function looks as the uses of the LHS of STMT and verifies they're all in the same block as STMT, right? It also verifies that the none of the operands within STMT are part of SSA_NAMES_SET. What role do those properties play in the meaning of local definition? I tried to explain it more deeply what's the purpose of this function. Thanks. It's much clearer now. We've actually got similar code in a couple places (ifcvt). I wonder if we could unify those implementations as a follow-up cleanup. Good point, I'm going to write it to my TODO list. @@ -1037,4 +1205,60 @@ func_checker::compare_gimple_asm (const gasm *g1, const gasm *g2) return true; } +void +ssa_names_set::build (basic_block bb) Needs a function comment. What are the important names we're collecting here? Is a single forward and backward pass really sufficient to find all the important names? In the backward pass, do you have to consider things like ASMs? I guess it's difficult to understand what you need to look at because it's not entirely clear the set of SSA_NAMEs you're building. These questions and lack of function comment don't seem to have been addressed yet. Fixed. + +using namespace ipa_icf; +using namespace ipa_icf_gimple; Is this wise? Does it significantly help with conciseness within the tail merging pass where it wants things ipa-icf and ipa-icf-gimple? I'm not objecting at this point, I want to hear your thoughts. I must agree with you, as I've started using both namespaces in tree-tail merge pass, it makes not much sense anymore. I suggest to come up with a namespace that will encapsulate 'identical code folding'-related stuff. What about: namespace icf Sure if it helps and is clean. GCC does not have a policy against using namespace, but other codebases do (google for example) as it does introduce some long term maintenance issues. So when I see a using namespace of that nature, I'm naturally going to question if it really helps in a significant way. If it does, then I won't object. If it's not helping in a significant way, then I'm likely to object. I think the updated version is fine WRT namespaces. Sound good! ? /* Describes a group of bbs with the same successors. The successor bbs are cached in succs, and the successor edge flags are cached in succ_flags. @@ -1220,17 +1231,48 @@ gsi_advance_bw_nondebug_nonlocal (gimple_stmt_iterator *gsi, tree *vuse, } } +static bool +check_edges_correspondence (basic_block bb1, basic_block bb2) Needs a function comment. Still needs function comment. I think we're pretty close here. Most of the issues are comments -- I still haven't looked *real* close at ssa_names_set::build. With a function comment I ought to be able to look at it more closely in the next (and hopefully final) iteration. Comment in this part was enhanced. The patch can boostrap on x86_64-linux-gnu, ppcl64-linux-gnu and survives regression tests on x86_64-linux-gnu. Thanks, Martin Jeff From f72d05044f9d8a77b0d2c3df68eba4f88824d08b Mon Sep 17 00:00:00 2001 From: mliska mli...@suse.cz Date: Thu, 9 Jul 2015 15:56:34 +0200 Subject: [PATCH 1/2] tree-ssa-tail-merge: replace
Re: [PATCH 1/3] tree-ssa-tail-merge: add IPA ICF infrastructure.
PING. Thanks, Martin On 07/20/2015 09:51 AM, Martin Liška wrote: On 07/16/2015 01:03 PM, Martin Liška wrote: On 07/09/2015 06:24 PM, Jeff Law wrote: On 07/09/2015 07:56 AM, mliska wrote: gcc/ChangeLog: 2015-07-09 Martin Liska mli...@suse.cz * dbgcnt.def: Add new debug counter. * ipa-icf-gimple.c (func_checker::compare_ssa_name): Add flag for strict mode. (func_checker::compare_memory_operand): Likewise. (func_checker::compare_cst_or_decl): Handle if we are in tail_merge_mode. (func_checker::compare_operand): Pass strict flag properly. (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): Pass strict flag. (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. * ipa-icf.h: Add new declarations. * 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. So a general question. We're passing in STRICT to several routines, which is fine. But then we're also checking M_TAIL_MERGE_MODE. What's the difference between the two? Can they be unified? Hello. I would say that STRICT is a bit generic mechanism that was introduced some time before. It's e.g. used for checking of THIS arguments for methods and make checking more sensitive in situations that are somehow special. The newly added state is orthogonal to the previous one. -/* Verifies that trees T1 and T2 are equivalent from perspective of ICF. */ +/* Verifies that trees T1 and T2 are equivalent from perspective of ICF. + If STRICT flag is true, versions must match strictly. */ bool -func_checker::compare_ssa_name (tree t1, tree t2) +func_checker::compare_ssa_name (tree t1, tree t2, bool strict) This (and other) functions would seem to be used more than just ICF at this point. A pass over the comments to update them as appropriate would be appreciated. @@ -626,6 +648,136 @@ func_checker::parse_labels (sem_bb *bb) } } +/* Return true if gimple STMT is just a local difinition in a + basic block. Used SSA names are contained in SSA_NAMES_SET. */ s/difinition/definition/ Thanks. I didn't find this comment particularly useful in understanding what this function does. AFAICT the function looks as the uses of the LHS of STMT and verifies they're all in the same block as STMT, right? It also verifies that the none of the operands within STMT are part of SSA_NAMES_SET. What role do those properties play in the meaning of local definition? I tried to explain it more deeply what's the purpose of this function. @@ -1037,4 +1205,60 @@ func_checker::compare_gimple_asm (const gasm *g1, const gasm *g2) return true; } +void +ssa_names_set::build (basic_block bb) Needs a function comment. What are the important names we're collecting here? Is a single forward and backward pass really sufficient to find all the important names? In the backward pass, do you have to consider things like ASMs? I guess it's difficult to understand what you need to look at because it's not entirely clear the set of SSA_NAMEs you're building. @@ -149,12 +153,20 @@ public: mapping between basic blocks and labels. */ void parse_labels (sem_bb *bb); + /* For given basic blocks BB1 and BB2 (from functions FUNC1 and FUNC), + true value is returned if phi nodes are semantically + equivalent in these blocks. */ + bool compare_phi_node (sem_bb *sem_bb1, sem_bb *sem_bb2); Presumably in the case of tail merging, FUNC1 and FUNC will be the same :-) Yes, the function is not called from tail-merge pass. /* Verifies that trees T1 and T2 are equivalent from perspective of ICF. */ - bool compare_ssa_name (tree t1, tree t2); + bool compare_ssa_name (tree t1, tree t2, bool strict = true); /* Verification function for edges E1 and E2. */ bool compare_edge (edge e1, edge e2); @@ -204,7 +216,7 @@ public: bool compare_tree_ssa_label (tree t1, tree t2); /* Function compare for equality given memory operands T1 and T2. */ - bool compare_memory_operand (tree t1, tree t2); + bool
Re: [PATCH 1/3] tree-ssa-tail-merge: add IPA ICF infrastructure.
On 07/16/2015 05:03 AM, Martin Liška wrote: So a general question. We're passing in STRICT to several routines, which is fine. But then we're also checking M_TAIL_MERGE_MODE. What's the difference between the two? Can they be unified? Hello. I would say that STRICT is a bit generic mechanism that was introduced some time before. It's e.g. used for checking of THIS arguments for methods and make checking more sensitive in situations that are somehow special. The newly added state is orthogonal to the previous one. Fair enough. There's some cases where we've documented STRICT, and others where we haven't. If STRICT flag is true, version must match strictly Appears as documentation for STRICT. It seems like it'd be better to describe what strictly means here. Elsewhere we have comments like: Be strict in case of tail-merge optimization Which tends to confuse things a bit. Perhaps something more like: In the case of tail merge optimization, XYZ must match It seems like a nit, but to someone else reading the code I don't think the distinctions are all that clear right now, so if we can improve things, it'd be good. I didn't find this comment particularly useful in understanding what this function does. AFAICT the function looks as the uses of the LHS of STMT and verifies they're all in the same block as STMT, right? It also verifies that the none of the operands within STMT are part of SSA_NAMES_SET. What role do those properties play in the meaning of local definition? I tried to explain it more deeply what's the purpose of this function. Thanks. It's much clearer now. We've actually got similar code in a couple places (ifcvt). I wonder if we could unify those implementations as a follow-up cleanup. @@ -1037,4 +1205,60 @@ func_checker::compare_gimple_asm (const gasm *g1, const gasm *g2) return true; } +void +ssa_names_set::build (basic_block bb) Needs a function comment. What are the important names we're collecting here? Is a single forward and backward pass really sufficient to find all the important names? In the backward pass, do you have to consider things like ASMs? I guess it's difficult to understand what you need to look at because it's not entirely clear the set of SSA_NAMEs you're building. These questions and lack of function comment don't seem to have been addressed yet. + +using namespace ipa_icf; +using namespace ipa_icf_gimple; Is this wise? Does it significantly help with conciseness within the tail merging pass where it wants things ipa-icf and ipa-icf-gimple? I'm not objecting at this point, I want to hear your thoughts. I must agree with you, as I've started using both namespaces in tree-tail merge pass, it makes not much sense anymore. I suggest to come up with a namespace that will encapsulate 'identical code folding'-related stuff. What about: namespace icf Sure if it helps and is clean. GCC does not have a policy against using namespace, but other codebases do (google for example) as it does introduce some long term maintenance issues. So when I see a using namespace of that nature, I'm naturally going to question if it really helps in a significant way. If it does, then I won't object. If it's not helping in a significant way, then I'm likely to object. I think the updated version is fine WRT namespaces. ? /* Describes a group of bbs with the same successors. The successor bbs are cached in succs, and the successor edge flags are cached in succ_flags. @@ -1220,17 +1231,48 @@ gsi_advance_bw_nondebug_nonlocal (gimple_stmt_iterator *gsi, tree *vuse, } } +static bool +check_edges_correspondence (basic_block bb1, basic_block bb2) Needs a function comment. Still needs function comment. I think we're pretty close here. Most of the issues are comments -- I still haven't looked *real* close at ssa_names_set::build. With a function comment I ought to be able to look at it more closely in the next (and hopefully final) iteration. Jeff
Re: [PATCH 1/3] tree-ssa-tail-merge: add IPA ICF infrastructure.
On 07/20/2015 01:52 AM, Martin Liška wrote: Due to changes dump file, it will be necessary to amend scanning of dump files. Those changes are fine and can be installed once the prerequisites have been approved/installed. Thanks, Jeff
Re: [PATCH 1/3] tree-ssa-tail-merge: add IPA ICF infrastructure.
On 07/16/2015 01:03 PM, Martin Liška wrote: On 07/09/2015 06:24 PM, Jeff Law wrote: On 07/09/2015 07:56 AM, mliska wrote: gcc/ChangeLog: 2015-07-09 Martin Liska mli...@suse.cz * dbgcnt.def: Add new debug counter. * ipa-icf-gimple.c (func_checker::compare_ssa_name): Add flag for strict mode. (func_checker::compare_memory_operand): Likewise. (func_checker::compare_cst_or_decl): Handle if we are in tail_merge_mode. (func_checker::compare_operand): Pass strict flag properly. (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): Pass strict flag. (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. * ipa-icf.h: Add new declarations. * 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. So a general question. We're passing in STRICT to several routines, which is fine. But then we're also checking M_TAIL_MERGE_MODE. What's the difference between the two? Can they be unified? Hello. I would say that STRICT is a bit generic mechanism that was introduced some time before. It's e.g. used for checking of THIS arguments for methods and make checking more sensitive in situations that are somehow special. The newly added state is orthogonal to the previous one. -/* Verifies that trees T1 and T2 are equivalent from perspective of ICF. */ +/* Verifies that trees T1 and T2 are equivalent from perspective of ICF. + If STRICT flag is true, versions must match strictly. */ bool -func_checker::compare_ssa_name (tree t1, tree t2) +func_checker::compare_ssa_name (tree t1, tree t2, bool strict) This (and other) functions would seem to be used more than just ICF at this point. A pass over the comments to update them as appropriate would be appreciated. @@ -626,6 +648,136 @@ func_checker::parse_labels (sem_bb *bb) } } +/* Return true if gimple STMT is just a local difinition in a + basic block. Used SSA names are contained in SSA_NAMES_SET. */ s/difinition/definition/ Thanks. I didn't find this comment particularly useful in understanding what this function does. AFAICT the function looks as the uses of the LHS of STMT and verifies they're all in the same block as STMT, right? It also verifies that the none of the operands within STMT are part of SSA_NAMES_SET. What role do those properties play in the meaning of local definition? I tried to explain it more deeply what's the purpose of this function. @@ -1037,4 +1205,60 @@ func_checker::compare_gimple_asm (const gasm *g1, const gasm *g2) return true; } +void +ssa_names_set::build (basic_block bb) Needs a function comment. What are the important names we're collecting here? Is a single forward and backward pass really sufficient to find all the important names? In the backward pass, do you have to consider things like ASMs? I guess it's difficult to understand what you need to look at because it's not entirely clear the set of SSA_NAMEs you're building. @@ -149,12 +153,20 @@ public: mapping between basic blocks and labels. */ void parse_labels (sem_bb *bb); + /* For given basic blocks BB1 and BB2 (from functions FUNC1 and FUNC), + true value is returned if phi nodes are semantically + equivalent in these blocks. */ + bool compare_phi_node (sem_bb *sem_bb1, sem_bb *sem_bb2); Presumably in the case of tail merging, FUNC1 and FUNC will be the same :-) Yes, the function is not called from tail-merge pass. /* Verifies that trees T1 and T2 are equivalent from perspective of ICF. */ - bool compare_ssa_name (tree t1, tree t2); + bool compare_ssa_name (tree t1, tree t2, bool strict = true); /* Verification function for edges E1 and E2. */ bool compare_edge (edge e1, edge e2); @@ -204,7 +216,7 @@ public: bool compare_tree_ssa_label (tree t1, tree t2); /* Function compare for equality given memory operands T1 and T2. */ - bool compare_memory_operand (tree t1, tree t2); + bool compare_memory_operand (tree t1, tree t2, bool strict = true); /*
Re: [PATCH 1/3] tree-ssa-tail-merge: add IPA ICF infrastructure.
On 07/16/2015 01:03 PM, Martin Liška wrote: On 07/09/2015 06:24 PM, Jeff Law wrote: On 07/09/2015 07:56 AM, mliska wrote: gcc/ChangeLog: 2015-07-09 Martin Liska mli...@suse.cz * dbgcnt.def: Add new debug counter. * ipa-icf-gimple.c (func_checker::compare_ssa_name): Add flag for strict mode. (func_checker::compare_memory_operand): Likewise. (func_checker::compare_cst_or_decl): Handle if we are in tail_merge_mode. (func_checker::compare_operand): Pass strict flag properly. (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): Pass strict flag. (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. * ipa-icf.h: Add new declarations. * 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. So a general question. We're passing in STRICT to several routines, which is fine. But then we're also checking M_TAIL_MERGE_MODE. What's the difference between the two? Can they be unified? Hello. I would say that STRICT is a bit generic mechanism that was introduced some time before. It's e.g. used for checking of THIS arguments for methods and make checking more sensitive in situations that are somehow special. The newly added state is orthogonal to the previous one. -/* Verifies that trees T1 and T2 are equivalent from perspective of ICF. */ +/* Verifies that trees T1 and T2 are equivalent from perspective of ICF. + If STRICT flag is true, versions must match strictly. */ bool -func_checker::compare_ssa_name (tree t1, tree t2) +func_checker::compare_ssa_name (tree t1, tree t2, bool strict) This (and other) functions would seem to be used more than just ICF at this point. A pass over the comments to update them as appropriate would be appreciated. @@ -626,6 +648,136 @@ func_checker::parse_labels (sem_bb *bb) } } +/* Return true if gimple STMT is just a local difinition in a + basic block. Used SSA names are contained in SSA_NAMES_SET. */ s/difinition/definition/ Thanks. I didn't find this comment particularly useful in understanding what this function does. AFAICT the function looks as the uses of the LHS of STMT and verifies they're all in the same block as STMT, right? It also verifies that the none of the operands within STMT are part of SSA_NAMES_SET. What role do those properties play in the meaning of local definition? I tried to explain it more deeply what's the purpose of this function. @@ -1037,4 +1205,60 @@ func_checker::compare_gimple_asm (const gasm *g1, const gasm *g2) return true; } +void +ssa_names_set::build (basic_block bb) Needs a function comment. What are the important names we're collecting here? Is a single forward and backward pass really sufficient to find all the important names? In the backward pass, do you have to consider things like ASMs? I guess it's difficult to understand what you need to look at because it's not entirely clear the set of SSA_NAMEs you're building. @@ -149,12 +153,20 @@ public: mapping between basic blocks and labels. */ void parse_labels (sem_bb *bb); + /* For given basic blocks BB1 and BB2 (from functions FUNC1 and FUNC), + true value is returned if phi nodes are semantically + equivalent in these blocks. */ + bool compare_phi_node (sem_bb *sem_bb1, sem_bb *sem_bb2); Presumably in the case of tail merging, FUNC1 and FUNC will be the same :-) Yes, the function is not called from tail-merge pass. /* Verifies that trees T1 and T2 are equivalent from perspective of ICF. */ - bool compare_ssa_name (tree t1, tree t2); + bool compare_ssa_name (tree t1, tree t2, bool strict = true); /* Verification function for edges E1 and E2. */ bool compare_edge (edge e1, edge e2); @@ -204,7 +216,7 @@ public: bool compare_tree_ssa_label (tree t1, tree t2); /* Function compare for equality given memory operands T1 and T2. */ - bool compare_memory_operand (tree t1, tree t2); + bool compare_memory_operand (tree t1, tree t2, bool strict = true); /*
Re: [PATCH 1/3] tree-ssa-tail-merge: add IPA ICF infrastructure.
On 07/09/2015 06:24 PM, Jeff Law wrote: On 07/09/2015 07:56 AM, mliska wrote: gcc/ChangeLog: 2015-07-09 Martin Liska mli...@suse.cz * dbgcnt.def: Add new debug counter. * ipa-icf-gimple.c (func_checker::compare_ssa_name): Add flag for strict mode. (func_checker::compare_memory_operand): Likewise. (func_checker::compare_cst_or_decl): Handle if we are in tail_merge_mode. (func_checker::compare_operand): Pass strict flag properly. (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): Pass strict flag. (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. * ipa-icf.h: Add new declarations. * 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. So a general question. We're passing in STRICT to several routines, which is fine. But then we're also checking M_TAIL_MERGE_MODE. What's the difference between the two? Can they be unified? Hello. I would say that STRICT is a bit generic mechanism that was introduced some time before. It's e.g. used for checking of THIS arguments for methods and make checking more sensitive in situations that are somehow special. The newly added state is orthogonal to the previous one. -/* Verifies that trees T1 and T2 are equivalent from perspective of ICF. */ +/* Verifies that trees T1 and T2 are equivalent from perspective of ICF. + If STRICT flag is true, versions must match strictly. */ bool -func_checker::compare_ssa_name (tree t1, tree t2) +func_checker::compare_ssa_name (tree t1, tree t2, bool strict) This (and other) functions would seem to be used more than just ICF at this point. A pass over the comments to update them as appropriate would be appreciated. @@ -626,6 +648,136 @@ func_checker::parse_labels (sem_bb *bb) } } +/* Return true if gimple STMT is just a local difinition in a + basic block. Used SSA names are contained in SSA_NAMES_SET. */ s/difinition/definition/ Thanks. I didn't find this comment particularly useful in understanding what this function does. AFAICT the function looks as the uses of the LHS of STMT and verifies they're all in the same block as STMT, right? It also verifies that the none of the operands within STMT are part of SSA_NAMES_SET. What role do those properties play in the meaning of local definition? I tried to explain it more deeply what's the purpose of this function. @@ -1037,4 +1205,60 @@ func_checker::compare_gimple_asm (const gasm *g1, const gasm *g2) return true; } +void +ssa_names_set::build (basic_block bb) Needs a function comment. What are the important names we're collecting here? Is a single forward and backward pass really sufficient to find all the important names? In the backward pass, do you have to consider things like ASMs? I guess it's difficult to understand what you need to look at because it's not entirely clear the set of SSA_NAMEs you're building. @@ -149,12 +153,20 @@ public: mapping between basic blocks and labels. */ void parse_labels (sem_bb *bb); + /* For given basic blocks BB1 and BB2 (from functions FUNC1 and FUNC), + true value is returned if phi nodes are semantically + equivalent in these blocks. */ + bool compare_phi_node (sem_bb *sem_bb1, sem_bb *sem_bb2); Presumably in the case of tail merging, FUNC1 and FUNC will be the same :-) Yes, the function is not called from tail-merge pass. /* Verifies that trees T1 and T2 are equivalent from perspective of ICF. */ - bool compare_ssa_name (tree t1, tree t2); + bool compare_ssa_name (tree t1, tree t2, bool strict = true); /* Verification function for edges E1 and E2. */ bool compare_edge (edge e1, edge e2); @@ -204,7 +216,7 @@ public: bool compare_tree_ssa_label (tree t1, tree t2); /* Function compare for equality given memory operands T1 and T2. */ - bool compare_memory_operand (tree t1, tree t2); + bool compare_memory_operand (tree t1, tree t2, bool strict = true); /* Function compare for equality given trees T1 and
[PATCH 1/3] tree-ssa-tail-merge: add IPA ICF infrastructure.
gcc/ChangeLog:
2015-07-09 Martin Liska mli...@suse.cz
* dbgcnt.def: Add new debug counter.
* ipa-icf-gimple.c (func_checker::compare_ssa_name): Add flag
for strict mode.
(func_checker::compare_memory_operand): Likewise.
(func_checker::compare_cst_or_decl): Handle if we are in
tail_merge_mode.
(func_checker::compare_operand): Pass strict flag properly.
(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): Pass strict flag.
(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.
* ipa-icf.h: Add new declarations.
* 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.
---
gcc/dbgcnt.def| 1 +
gcc/ipa-icf-gimple.c | 272 ++
gcc/ipa-icf-gimple.h | 78 +++--
gcc/ipa-icf.c | 67 +---
gcc/ipa-icf.h | 14 ++-
gcc/tree-ssa-tail-merge.c | 87 +--
6 files changed, 407 insertions(+), 112 deletions(-)
diff --git a/gcc/dbgcnt.def b/gcc/dbgcnt.def
index 95f6b06..79d99d9 100644
--- a/gcc/dbgcnt.def
+++ b/gcc/dbgcnt.def
@@ -187,6 +187,7 @@ DEBUG_COUNTER (sms_sched_loop)
DEBUG_COUNTER (split_for_sched2)
DEBUG_COUNTER (store_motion)
DEBUG_COUNTER (tail_call)
+DEBUG_COUNTER (tail_merge)
DEBUG_COUNTER (treepre_insert)
DEBUG_COUNTER (tree_sra)
DEBUG_COUNTER (vect_loop)
diff --git a/gcc/ipa-icf-gimple.c b/gcc/ipa-icf-gimple.c
index e727693..df99c0d 100644
--- a/gcc/ipa-icf-gimple.c
+++ b/gcc/ipa-icf-gimple.c
@@ -54,6 +54,7 @@ along with GCC; see the file COPYING3. If not see
#include list
#include tree-eh.h
#include builtins.h
+#include trans-mem.h
#include ipa-icf-gimple.h
#include ipa-icf.h
@@ -69,14 +70,14 @@ namespace ipa_icf_gimple {
func_checker::func_checker (tree source_func_decl, tree target_func_decl,
bool compare_polymorphic,
- bool ignore_labels,
+ bool tail_merge_mode,
hash_setsymtab_node * *ignored_source_nodes,
hash_setsymtab_node * *ignored_target_nodes)
: m_source_func_decl (source_func_decl), m_target_func_decl
(target_func_decl),
m_ignored_source_nodes (ignored_source_nodes),
m_ignored_target_nodes (ignored_target_nodes),
m_compare_polymorphic (compare_polymorphic),
-m_ignore_labels (ignore_labels)
+m_tail_merge_mode (tail_merge_mode)
{
function *source_func = DECL_STRUCT_FUNCTION (source_func_decl);
function *target_func = DECL_STRUCT_FUNCTION (target_func_decl);
@@ -102,10 +103,11 @@ func_checker::~func_checker ()
m_target_ssa_names.release();
}
-/* Verifies that trees T1 and T2 are equivalent from perspective of ICF. */
+/* Verifies that trees T1 and T2 are equivalent from perspective of ICF.
+ If STRICT flag is true, versions must match strictly. */
bool
-func_checker::compare_ssa_name (tree t1, tree t2)
+func_checker::compare_ssa_name (tree t1, tree t2, bool strict)
{
gcc_assert (TREE_CODE (t1) == SSA_NAME);
gcc_assert (TREE_CODE (t2) == SSA_NAME);
@@ -113,6 +115,10 @@ func_checker::compare_ssa_name (tree t1, tree t2)
unsigned i1 = SSA_NAME_VERSION (t1);
unsigned i2 = SSA_NAME_VERSION (t2);
+ if (strict m_tail_merge_mode)
+return t1 == t2 ||
+ (m_source_ssa_names[i1] != -1 m_source_ssa_names[i1] == (int) i2);
+
if (m_source_ssa_names[i1] == -1)
m_source_ssa_names[i1] = i2;
else if (m_source_ssa_names[i1] != (int) i2)
@@ -256,10 +262,11 @@ 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. */
bool
-func_checker::compare_memory_operand (tree t1, tree t2)
+func_checker::compare_memory_operand (tree t1, tree t2, bool strict)
{
if (!t1 !t2)
return true;
@@ -327,7 +334,7 @@
Re: [PATCH 1/3] tree-ssa-tail-merge: add IPA ICF infrastructure.
On 07/09/2015 07:56 AM, mliska wrote: gcc/ChangeLog: 2015-07-09 Martin Liska mli...@suse.cz * dbgcnt.def: Add new debug counter. * ipa-icf-gimple.c (func_checker::compare_ssa_name): Add flag for strict mode. (func_checker::compare_memory_operand): Likewise. (func_checker::compare_cst_or_decl): Handle if we are in tail_merge_mode. (func_checker::compare_operand): Pass strict flag properly. (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): Pass strict flag. (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. * ipa-icf.h: Add new declarations. * 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. So a general question. We're passing in STRICT to several routines, which is fine. But then we're also checking M_TAIL_MERGE_MODE. What's the difference between the two? Can they be unified? -/* Verifies that trees T1 and T2 are equivalent from perspective of ICF. */ +/* Verifies that trees T1 and T2 are equivalent from perspective of ICF. + If STRICT flag is true, versions must match strictly. */ bool -func_checker::compare_ssa_name (tree t1, tree t2) +func_checker::compare_ssa_name (tree t1, tree t2, bool strict) This (and other) functions would seem to be used more than just ICF at this point. A pass over the comments to update them as appropriate would be appreciated. @@ -626,6 +648,136 @@ func_checker::parse_labels (sem_bb *bb) } } +/* Return true if gimple STMT is just a local difinition in a + basic block. Used SSA names are contained in SSA_NAMES_SET. */ s/difinition/definition/ I didn't find this comment particularly useful in understanding what this function does. AFAICT the function looks as the uses of the LHS of STMT and verifies they're all in the same block as STMT, right? It also verifies that the none of the operands within STMT are part of SSA_NAMES_SET. What role do those properties play in the meaning of local definition? @@ -1037,4 +1205,60 @@ func_checker::compare_gimple_asm (const gasm *g1, const gasm *g2) return true; } +void +ssa_names_set::build (basic_block bb) Needs a function comment. What are the important names we're collecting here? Is a single forward and backward pass really sufficient to find all the important names? In the backward pass, do you have to consider things like ASMs? I guess it's difficult to understand what you need to look at because it's not entirely clear the set of SSA_NAMEs you're building. @@ -149,12 +153,20 @@ public: mapping between basic blocks and labels. */ void parse_labels (sem_bb *bb); + /* For given basic blocks BB1 and BB2 (from functions FUNC1 and FUNC), + true value is returned if phi nodes are semantically + equivalent in these blocks. */ + bool compare_phi_node (sem_bb *sem_bb1, sem_bb *sem_bb2); Presumably in the case of tail merging, FUNC1 and FUNC will be the same :-) /* Verifies that trees T1 and T2 are equivalent from perspective of ICF. */ - bool compare_ssa_name (tree t1, tree t2); + bool compare_ssa_name (tree t1, tree t2, bool strict = true); /* Verification function for edges E1 and E2. */ bool compare_edge (edge e1, edge e2); @@ -204,7 +216,7 @@ public: bool compare_tree_ssa_label (tree t1, tree t2); /* Function compare for equality given memory operands T1 and T2. */ - bool compare_memory_operand (tree t1, tree t2); + bool compare_memory_operand (tree t1, tree t2, bool strict = true); /* Function compare for equality given trees T1 and T2 which can be either a constant or a declaration type. */ @@ -213,7 +225,7 @@ public: /* Function responsible for comparison of various operands T1 and T2. If these components, from functions FUNC1 and FUNC2, are equal, true is returned. */ - bool compare_operand (tree t1, tree t2); + bool compare_operand (tree t1, tree t2, bool strict = false); Is the default value for the parameter really needed in these
