Re: [PATCH, 1/6] Simplify constraint handling

[Tom de Vries <Tom_deVries@mentor.com>]

[-- Attachment #1: Type: text/plain, Size: 4231 bytes --]

On 29/10/15 14:12, Richard Biener wrote:
> On Thu, 29 Oct 2015, Tom de Vries wrote:
>
>> >On 29/10/15 12:13, Richard Biener wrote:
>>> > >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?
>> >
>> >First, there was an error in the patch, it tested for flag_ipa_pta (so it also
>> >affected ealias), but it was supposed to test for in_ipa mode. That is fixed
>> >in attached version.
>> >
>>> > >I don't see the patch simplifies anything but only removes spurious
>>> > >settings by adding IMHO redundant checks.
>> >
>> >Consider testcase:
>> >...
>> >int __attribute__((noinline, noclone))
>> >foo (int *__restrict__ a, int *__restrict__ b)
>> >{
>> >   *a = 1;
>> >   *b = 2;
>> >}
>> >
>> >int __attribute__((noinline, noclone))
>> >bar (int *a, int *b)
>> >{
>> >   foo (a, b);
>> >}
>> >...
>> >
>> >The impact of this patch in the pta dump (focusing on the constraints bit) is:
>> >...
>> >  Generating constraints for foo (foo)
>> >
>> >-foo.arg0 = &PARM_NOALIAS(20)
>> >-PARM_NOALIAS(20) = NONLOCAL
>> >-foo.arg1 = &PARM_NOALIAS(21)
>> >-PARM_NOALIAS(21) = NONLOCAL
>> >+foo.arg0 = &NONLOCAL
>> >+foo.arg1 = &NONLOCAL
>> >...
>> >
>> >That's the kind of simplifications I'm trying to achieve.
> Yes, but as I said we should refactor things to avoid calling
> the intra constraints generation from the IPA path.

Ah, I see.

So, like this? OK for trunk if bootstrap and reg-test succeeds?

Thanks,
- Tom


[-- Attachment #2: 0012-Add-initial-constraints-in-create_function_info_for.patch --]
[-- Type: text/x-patch, Size: 5084 bytes --]

Add initial constraints in create_function_info_for

2015-10-29  Tom de Vries  <tom@codesourcery.com>

	* tree-ssa-structalias.c (ipa_pta_execute): Add extra arg to call to
	create_function_info_for.  Dump constraints generated during
	create_function_info_for. Move intra_create_variable_infos call and
	function-return-values-escape bit to ...
	(create_function_info_for): ... here, and merge
	intra_create_variable_infos call with argument loop.  Add and handle
	nonlocal_p parameter.
---
 gcc/tree-ssa-structalias.c | 94 ++++++++++++++++++++++++++++------------------
 1 file changed, 58 insertions(+), 36 deletions(-)

diff --git a/gcc/tree-ssa-structalias.c b/gcc/tree-ssa-structalias.c
index 3bef607..3a0891c 100644
--- a/gcc/tree-ssa-structalias.c
+++ b/gcc/tree-ssa-structalias.c
@@ -5421,10 +5421,12 @@ count_num_arguments (tree decl, bool *is_varargs)
 }
 
 /* Creation function node for DECL, using NAME, and return the index
-   of the variable we've created for the function.  */
+   of the variable we've created for the function.  If NONLOCAL_p, create
+   initial constraints.  */
 
 static varinfo_t
-create_function_info_for (tree decl, const char *name, bool add_id)
+create_function_info_for (tree decl, const char *name, bool add_id,
+			  bool nonlocal_p)
 {
   struct function *fn = DECL_STRUCT_FUNCTION (decl);
   varinfo_t vi, prev_vi;
@@ -5536,6 +5538,28 @@ create_function_info_for (tree decl, const char *name, bool add_id)
 	insert_vi_for_tree (DECL_RESULT (decl), resultvi);
     }
 
+  /* We also need to make function return values escape.  Nothing
+     escapes by returning from main though.  */
+  if (nonlocal_p
+      && !MAIN_NAME_P (DECL_NAME (decl)))
+    {
+      varinfo_t fi, rvi;
+      fi = lookup_vi_for_tree (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));
+	}
+    }
+
   /* Set up variables for each argument.  */
   arg = DECL_ARGUMENTS (decl);
   for (i = 0; i < num_args; i++)
@@ -5562,6 +5586,11 @@ create_function_info_for (tree decl, const char *name, bool add_id)
       gcc_assert (prev_vi->offset < argvi->offset);
       prev_vi->next = argvi->id;
       prev_vi = argvi;
+
+      if (nonlocal_p
+	  && argvi->may_have_pointers)
+	make_constraint_from (argvi, nonlocal_id);
+
       if (arg)
 	{
 	  insert_vi_for_tree (arg, argvi);
@@ -7317,8 +7346,34 @@ ipa_pta_execute (void)
 
       gcc_assert (!node->clone_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.  */
+      bool nonlocal_p = (node->used_from_other_partition
+			 || node->externally_visible
+			 || node->force_output
+			 || node->address_taken);
+
       vi = create_function_info_for (node->decl,
-				     alias_get_name (node->decl), false);
+				     alias_get_name (node->decl), false,
+				     nonlocal_p);
+      if (dump_file
+	  && from != constraints.length ())
+	{
+	  fprintf (dump_file,
+		   "Generating intial constraints for %s", node->name ());
+	  if (DECL_ASSEMBLER_NAME_SET_P (node->decl))
+	    fprintf (dump_file, " (%s)",
+		     IDENTIFIER_POINTER
+		       (DECL_ASSEMBLER_NAME (node->decl)));
+	  fprintf (dump_file, "\n\n");
+	  dump_constraints (dump_file, from);
+	  fprintf (dump_file, "\n");
+
+	  from = constraints.length ();
+	}
+
       node->call_for_symbol_thunks_and_aliases
 	(associate_varinfo_to_alias, vi, true);
     }
@@ -7365,39 +7420,6 @@ ipa_pta_execute (void)
       func = DECL_STRUCT_FUNCTION (node->decl);
       gcc_assert (cfun == NULL);
 
-      /* 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));
-		}
-	    }
-	}
-
       /* Build constriants for the function body.  */
       FOR_EACH_BB_FN (bb, func)
 	{
-- 
1.9.1