| Message ID | 22214476.EfDdHjke4D@localhost.localdomain |
|---|---|
| State | New |
| Headers | show |
| Series | Fix PR ipa/113996 | expand |
On 3/11/24 4:38 PM, Eric Botcazou wrote: > Hi, > > this is a regression present on all active branches: the attached Ada testcase > triggers an assertion failure when compiled with -O2 -gnatp -flto: > > /* Initialize the static chain. */ > p = DECL_STRUCT_FUNCTION (fn)->static_chain_decl; > gcc_assert (fn != current_function_decl); > if (p) > { > /* No static chain? Seems like a bug in tree-nested.cc. */ > gcc_assert (static_chain); <--- here > > setup_one_parameter (id, p, static_chain, fn, bb, &vars); > } > > The problem is that the ICF pass identifies two functions, one of which has a > static chain but the other does not. The proposed fix is just to prevent this > identification from occurring. > > Tested on x86-64/Linux, OK for all active branches? > > > 2024-03-11 Eric Botcazou <ebotcazou@adacore.com> > > PR ipa/113996 > * ipa-icf.h (sem_function): Add static_chain_present member. > * ipa-icf.cc (sem_function::get_hash): Hash it. > (sem_function::equals_wpa): Compare it. > (sem_function::equals_private): Likewise. > (sem_function::init): Initialize it. > > > 2024-03-11 Eric Botcazou <ebotcazou@adacore.com> > > * gnat.dg/lto27.adb: New test. OK. jeff
> > > On 3/11/24 4:38 PM, Eric Botcazou wrote: > > Hi, > > > > this is a regression present on all active branches: the attached Ada testcase > > triggers an assertion failure when compiled with -O2 -gnatp -flto: > > > > /* Initialize the static chain. */ > > p = DECL_STRUCT_FUNCTION (fn)->static_chain_decl; > > gcc_assert (fn != current_function_decl); > > if (p) > > { > > /* No static chain? Seems like a bug in tree-nested.cc. */ > > gcc_assert (static_chain); <--- here > > > > setup_one_parameter (id, p, static_chain, fn, bb, &vars); > > } > > > > The problem is that the ICF pass identifies two functions, one of which has a > > static chain but the other does not. The proposed fix is just to prevent this > > identification from occurring. > > > > Tested on x86-64/Linux, OK for all active branches? > > > > > > 2024-03-11 Eric Botcazou <ebotcazou@adacore.com> > > > > PR ipa/113996 > > * ipa-icf.h (sem_function): Add static_chain_present member. > > * ipa-icf.cc (sem_function::get_hash): Hash it. > > (sem_function::equals_wpa): Compare it. > > (sem_function::equals_private): Likewise. > > (sem_function::init): Initialize it. > > > > > > 2024-03-11 Eric Botcazou <ebotcazou@adacore.com> > > > > * gnat.dg/lto27.adb: New test. > OK. Patch is still OK, but ipa-ICF will only identify the functions if static chain is unused. Perhaps just picking the winning candidate to be version without static chain and making ipa-inline to not ICE when calls with static chain lands to function with no static chain would help us to optimize better. Also IPA-SRA can optimize out unused static chains and ipa-prop can propagate across them. Honza > jeff >
> Patch is still OK, but ipa-ICF will only identify the functions if > static chain is unused. Perhaps just picking the winning candidate to be > version without static chain and making ipa-inline to not ICE when calls > with static chain lands to function with no static chain would help us > to optimize better. I see, thanks for the explanation. The attached patch appears to work. PR ipa/113996 * ipa-icf.h (sem_function): Add static_chain_p member. * ipa-icf.cc (sem_function::init): Initialize it. (sem_item_optimizer::merge_classes): If the class is made of functions, pick one without static chain as the target.
> > Patch is still OK, but ipa-ICF will only identify the functions if > > static chain is unused. Perhaps just picking the winning candidate to be > > version without static chain and making ipa-inline to not ICE when calls > > with static chain lands to function with no static chain would help us > > to optimize better. > > I see, thanks for the explanation. The attached patch appears to work. > > > PR ipa/113996 > * ipa-icf.h (sem_function): Add static_chain_p member. > * ipa-icf.cc (sem_function::init): Initialize it. > (sem_item_optimizer::merge_classes): If the class is made of > functions, pick one without static chain as the target. > > -- > Eric Botcazou > @@ -3399,11 +3401,22 @@ sem_item_optimizer::merge_classes (unsigned int prev_class_count, > > sem_item *source = c->members[0]; > > - if (DECL_NAME (source->decl) > - && MAIN_NAME_P (DECL_NAME (source->decl))) > - /* If merge via wrappers, picking main as the target can be > - problematic. */ > - source = c->members[1]; > + if (source->type == FUNC) > + { > + /* Pick a member without static chain, if any. */ > + for (unsigned int j = 0; j < c->members.length (); j++) > + if (!static_cast<sem_function *> (c->members[j])->static_chain_p) > + { > + source = c->members[j]; > + break; > + } > + > + /* If merge via wrappers, picking main as the target can be > + problematic. */ > + if (DECL_NAME (source->decl) > + && MAIN_NAME_P (DECL_NAME (source->decl))) > + source = c->members[source == c->members[0] ? 1 : 0]; Thanks for looking into this. I wonder if it can happen that we ICF merge function with static chain and main? We can fix this unlikely case by simply considering functions with static chain not equivalent to MAIN_NAME_P function. On x86_64 static chain goes to R10, but I wonder if we support targets where API of function taking static chain and not using it differs from API of function with no static chain? Honza
diff --git a/gcc/ipa-icf.cc b/gcc/ipa-icf.cc index 5d5a42f9c6c..dff7ad6efda 100644 --- a/gcc/ipa-icf.cc +++ b/gcc/ipa-icf.cc @@ -284,6 +284,7 @@ sem_function::get_hash (void) hstate.add_int (177454); /* Random number for function type. */ hstate.add_int (arg_count); + hstate.add_int (static_chain_present); hstate.add_int (cfg_checksum); hstate.add_int (gcode_hash); @@ -655,7 +656,10 @@ sem_function::equals_wpa (sem_item *item, } if (list1 || list2) - return return_false_with_msg ("Mismatched number of parameters"); + return return_false_with_msg ("mismatched number of parameters"); + + if (static_chain_present != m_compared_func->static_chain_present) + return return_false_with_msg ("static chain mismatch"); if (node->num_references () != item->node->num_references ()) return return_false_with_msg ("different number of references"); @@ -876,7 +880,10 @@ sem_function::equals_private (sem_item *item) return return_false (); } if (arg1 || arg2) - return return_false_with_msg ("Mismatched number of arguments"); + return return_false_with_msg ("mismatched number of arguments"); + + if (static_chain_present != m_compared_func->static_chain_present) + return return_false_with_msg ("static chain mismatch"); if (!dyn_cast <cgraph_node *> (node)->has_gimple_body_p ()) return true; @@ -1368,6 +1375,8 @@ sem_function::init (ipa_icf_gimple::func_checker *checker) /* iterating all function arguments. */ arg_count = count_formal_params (fndecl); + static_chain_present = func->static_chain_decl != NULL_TREE; + edge_count = n_edges_for_fn (func); cgraph_node *cnode = dyn_cast <cgraph_node *> (node); if (!cnode->thunk) diff --git a/gcc/ipa-icf.h b/gcc/ipa-icf.h index ef7e41bfa88..bd9fd9fb294 100644 --- a/gcc/ipa-icf.h +++ b/gcc/ipa-icf.h @@ -355,6 +355,9 @@ public: parameters. */ bool compatible_parm_types_p (tree, tree); + /* Return true if parameter I may be used. */ + bool param_used_p (unsigned int i); + /* Exception handling region tree. */ eh_region region_tree; @@ -379,6 +382,9 @@ public: /* Total number of SSA names used in the function. */ unsigned ssa_names_size; + /* Whether the special PARM_DECL for the static chain is present. */ + bool static_chain_present; + /* Array of structures for all basic blocks. */ vec <ipa_icf_gimple::sem_bb *> bb_sorted; @@ -386,9 +392,6 @@ public: function. */ hashval_t m_alias_sets_hash; - /* Return true if parameter I may be used. */ - bool param_used_p (unsigned int i); - private: /* Calculates hash value based on a BASIC_BLOCK. */ hashval_t get_bb_hash (const ipa_icf_gimple::sem_bb *basic_block);
Hi, this is a regression present on all active branches: the attached Ada testcase triggers an assertion failure when compiled with -O2 -gnatp -flto: /* Initialize the static chain. */ p = DECL_STRUCT_FUNCTION (fn)->static_chain_decl; gcc_assert (fn != current_function_decl); if (p) { /* No static chain? Seems like a bug in tree-nested.cc. */ gcc_assert (static_chain); <--- here setup_one_parameter (id, p, static_chain, fn, bb, &vars); } The problem is that the ICF pass identifies two functions, one of which has a static chain but the other does not. The proposed fix is just to prevent this identification from occurring. Tested on x86-64/Linux, OK for all active branches? 2024-03-11 Eric Botcazou <ebotcazou@adacore.com> PR ipa/113996 * ipa-icf.h (sem_function): Add static_chain_present member. * ipa-icf.cc (sem_function::get_hash): Hash it. (sem_function::equals_wpa): Compare it. (sem_function::equals_private): Likewise. (sem_function::init): Initialize it. 2024-03-11 Eric Botcazou <ebotcazou@adacore.com> * gnat.dg/lto27.adb: New test.