| Message ID | 5630F242.7040806@mentor.com |
|---|---|
| State | New |
| Headers | show |
On 28/10/15 17:05, Tom de Vries wrote: > On 28/10/15 16:35, Richard Biener wrote: >> On Tue, 27 Oct 2015, Tom de Vries wrote: >> >>> On 27/10/15 13:24, Tom de Vries wrote: >>>> Thinking it over a bit more, I realized the constraint handling started >>>> to be messy. I've reworked the patch series to simplify that first. >>>> >>>> 1 Simplify constraint handling >>>> 2 Rename make_restrict_var_constraints to >>>> make_param_constraints >>>> 3 Add recursion to make_param_constraints >>>> 4 Add handle_param parameter to create_variable_info_for_1 >>>> 5 Handle recursive restrict pointer in >>>> create_variable_info_for_1 >>>> 6 Handle restrict struct fields recursively >>>> >>>> Currently doing bootstrap and regtest on x86_64. >>>> >>>> I'll repost the patch series in reply to this message. >>> >>> This patch gets rid of this bit of code in intra_create_variable_infos: >>> ... >>> if (restrict_pointer_p) >>> make_constraint_from_global_restrict (p, "PARM_RESTRICT"); >>> else >>> .. >>> >>> I already proposed to remove it here ( >>> https://gcc.gnu.org/ml/gcc-patches/2015-10/msg02426.html ) but there >>> is a >>> problem with that approach: It can happen that restrict_pointer_p is >>> true, but >>> p->only_restrict_pointers is false. This happens with fipa-pta, when >>> create_function_info_for created a varinfo for the parameter before >>> intra_create_variable_infos was called. >>> >>> The patch handles that case now by setting p->only_restrict_pointers. >> >> Hmm, but ... restrict only has an effect in non-IPA mode. > > Indeed, I also realized that by now. So I've committed the original, approved patch from https://gcc.gnu.org/ml/gcc-patches/2015-10/msg02426.html Thanks, - Tom
On Wed, 28 Oct 2015, Tom de Vries wrote: > On 28/10/15 16:35, Richard Biener wrote: > > On Tue, 27 Oct 2015, Tom de Vries wrote: > > > > > On 27/10/15 13:24, Tom de Vries wrote: > > > > Thinking it over a bit more, I realized the constraint handling started > > > > to be messy. I've reworked the patch series to simplify that first. > > > > > > > > 1 Simplify constraint handling > > > > 2 Rename make_restrict_var_constraints to > > > > make_param_constraints > > > > 3 Add recursion to make_param_constraints > > > > 4 Add handle_param parameter to create_variable_info_for_1 > > > > 5 Handle recursive restrict pointer in > > > > create_variable_info_for_1 > > > > 6 Handle restrict struct fields recursively > > > > > > > > Currently doing bootstrap and regtest on x86_64. > > > > > > > > I'll repost the patch series in reply to this message. > > > > > > This patch gets rid of this bit of code in intra_create_variable_infos: > > > ... > > > if (restrict_pointer_p) > > > make_constraint_from_global_restrict (p, "PARM_RESTRICT"); > > > else > > > .. > > > > > > I already proposed to remove it here ( > > > https://gcc.gnu.org/ml/gcc-patches/2015-10/msg02426.html ) but there is a > > > problem with that approach: It can happen that restrict_pointer_p is true, > > > but > > > p->only_restrict_pointers is false. This happens with fipa-pta, when > > > create_function_info_for created a varinfo for the parameter before > > > intra_create_variable_infos was called. > > > > > > The patch handles that case now by setting p->only_restrict_pointers. > > > > Hmm, but ... restrict only has an effect in non-IPA mode. > > Indeed, I also realized that by now. > > I wrote attached patch to make that explicit and simplify fipa-pta. > > OK for trunk if bootstrap and reg-test succeeds? I don't see the patch simplifies anything but only removes spurious settings by adding IMHO redundant checks. > > That we > > use intra_create_variable_infos in IPA mode is only done for correctness > > (to set up incoming NONLOCAL) for functions we do not see all callers of. > > > > Right. > > > Maybe we should fix that (in intra_create_variable_infos properly > > add constraints from NONLOCAL for all such functions). > > > > Aren't we are adding those constraints currently in > intra_create_variable_infos? Maybe you meant 'in ipa_pta_execute'? I meant create_function_info_for. Include all the effects of /* For externally visible or attribute used annotated functions use local constraints for their arguments. For local functions we see all callers and thus do not need initial constraints for parameters. */ if (node->used_from_other_partition || node->externally_visible || node->force_output || node->address_taken) { intra_create_variable_infos (func); /* We also need to make function return values escape. Nothing escapes by returning from main though. */ if (!MAIN_NAME_P (DECL_NAME (node->decl))) { varinfo_t fi, rvi; fi = lookup_vi_for_tree (node->decl); rvi = first_vi_for_offset (fi, fi_result); if (rvi && rvi->offset == fi_result) { struct constraint_expr includes; struct constraint_expr var; includes.var = escaped_id; includes.offset = 0; includes.type = SCALAR; var.var = rvi->id; var.offset = 0; var.type = SCALAR; process_constraint (new_constraint (includes, var)); } } } in it (just pass it a flag on whether the function is non-local). The effect of intra_create_variable_infos above was supposed to add a constraint from NONLOCAL on all vars (but the return value which instead needs to escape as done above). Richard. > Thanks, > - Tom > > >
Don't interpret restrict in fipa-pta 2015-10-28 Tom de Vries <tom@codesourcery.com> * tree-ssa-structalias.c (push_fields_onto_fieldstack) (create_variable_info_for_1, create_variable_info_for) (intra_create_variable_infos): Ignore restrict if !flag_ipa_pta. --- gcc/tree-ssa-structalias.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/gcc/tree-ssa-structalias.c b/gcc/tree-ssa-structalias.c index 06415a2..878e837 100644 --- a/gcc/tree-ssa-structalias.c +++ b/gcc/tree-ssa-structalias.c @@ -5383,7 +5383,8 @@ push_fields_onto_fieldstack (tree type, vec<fieldoff_s> *fieldstack, e.must_have_pointers = must_have_pointers_p; e.may_have_pointers = true; e.only_restrict_pointers - = (!has_unknown_size + = (!flag_ipa_pta + && !has_unknown_size && POINTER_TYPE_P (field_type) && TYPE_RESTRICT (field_type)); fieldstack->safe_push (e); @@ -5696,7 +5697,8 @@ create_variable_info_for_1 (tree decl, const char *name, bool add_id) vi->fullsize = tree_to_uhwi (declsize); vi->size = vi->fullsize; vi->is_full_var = true; - if (POINTER_TYPE_P (TREE_TYPE (decl)) + if (!flag_ipa_pta + && POINTER_TYPE_P (TREE_TYPE (decl)) && TYPE_RESTRICT (TREE_TYPE (decl))) vi->only_restrict_pointers = 1; fieldstack.release (); @@ -5766,9 +5768,10 @@ create_variable_info_for (tree decl, const char *name, bool add_id) continue; /* Mark global restrict qualified pointers. */ - if ((POINTER_TYPE_P (TREE_TYPE (decl)) - && TYPE_RESTRICT (TREE_TYPE (decl))) - || vi->only_restrict_pointers) + if (!flag_ipa_pta + && ((POINTER_TYPE_P (TREE_TYPE (decl)) + && TYPE_RESTRICT (TREE_TYPE (decl))) + || vi->only_restrict_pointers)) { varinfo_t rvi = make_constraint_from_global_restrict (vi, "GLOBAL_RESTRICT", @@ -5885,7 +5888,8 @@ intra_create_variable_infos (struct function *fn) passed-by-reference argument. */ for (t = DECL_ARGUMENTS (fn->decl); t; t = DECL_CHAIN (t)) { - bool restrict_pointer_p = (POINTER_TYPE_P (TREE_TYPE (t)) + bool restrict_pointer_p = (!flag_ipa_pta + && POINTER_TYPE_P (TREE_TYPE (t)) && TYPE_RESTRICT (TREE_TYPE (t))); bool recursive_restrict_p = (restrict_pointer_p -- 1.9.1