diff mbox

[1/6] Simplify constraint handling

Message ID 56320D9B.4070006@mentor.com
State New
Headers show

Commit Message

Tom de Vries Oct. 29, 2015, 12:14 p.m. UTC
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.

Thanks,
- Tom

Comments

Richard Biener Oct. 29, 2015, 1:12 p.m. UTC | #1
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.

Richard.

> Thanks,
> - Tom
> 
>
diff mbox

Patch

Don't interpret restrict in ipa_pta_execute

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 in_ipa_mode.
---
 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 6f72dd3..66ff8cb 100644
--- a/gcc/tree-ssa-structalias.c
+++ b/gcc/tree-ssa-structalias.c
@@ -5384,7 +5384,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
+		  = (!in_ipa_mode
+		     && !has_unknown_size
 		     && POINTER_TYPE_P (field_type)
 		     && TYPE_RESTRICT (field_type));
 		fieldstack->safe_push (e);
@@ -5697,7 +5698,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 (!in_ipa_mode
+	  && POINTER_TYPE_P (TREE_TYPE (decl))
 	  && TYPE_RESTRICT (TREE_TYPE (decl)))
 	vi->only_restrict_pointers = 1;
       fieldstack.release ();
@@ -5767,9 +5769,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 (!in_ipa_mode
+	  && ((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",
@@ -5886,7 +5889,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 = (!in_ipa_mode
+				 && POINTER_TYPE_P (TREE_TYPE (t))
 				 && TYPE_RESTRICT (TREE_TYPE (t)));
       bool recursive_restrict_p
 	= (restrict_pointer_p
-- 
1.9.1