Patchwork [PR,55334] Disable IPA-CP on restrict pointers to arrays

login
register
mail settings
Submitter Martin Jambor
Date Feb. 18, 2013, 8:06 p.m.
Message ID <20130218200623.GB19120@virgil.suse>
Download mbox | patch
Permalink /patch/221465/
State New
Headers show

Comments

Martin Jambor - Feb. 18, 2013, 8:06 p.m.
Hi,

after much pondering about PR 55334 I came to conclusion that no nice
fix to the regression could be introduced in stage4.  So for the sake
of the SPEC 200 benchmark I decided to cripple IPA-CP on restrict
pointers to arrays so that the restrict-ness of the memory references
which need to be vectorized is not lost.

I have tried various less aggressive tricks like only propagating the
one constant address that heuristics decided was profitable and not
all the others that came from the same calling contexts (and were
restrict) but then inlining came along and finished the deed with the
same consequences as IPA-CP had.

That only reinforced my feeling that the patch below is a spec hack
and that we ought to find a better way of preserving the restrict-ness
across IPA optimizations.  Therefore I will keep the bug open and
revert this patch after 4.8 is branched and stage1 opens.

Meanwhile, this passes bootstrap and testsuite on x86_64-linux.  OK
for trunk?

Thanks,

Martin


2013-02-07  Martin Jambor  <mjambor@suse.cz>

	PR tree-optimization/55334
	* ipa-cp.c (initialize_node_lattices): Disable IPA-CP through and to
	restricted pointers to arrays.
Richard Guenther - Feb. 19, 2013, 9:21 a.m.
On Mon, Feb 18, 2013 at 9:06 PM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> after much pondering about PR 55334 I came to conclusion that no nice
> fix to the regression could be introduced in stage4.  So for the sake
> of the SPEC 200 benchmark I decided to cripple IPA-CP on restrict
> pointers to arrays so that the restrict-ness of the memory references
> which need to be vectorized is not lost.
>
> I have tried various less aggressive tricks like only propagating the
> one constant address that heuristics decided was profitable and not
> all the others that came from the same calling contexts (and were
> restrict) but then inlining came along and finished the deed with the
> same consequences as IPA-CP had.
>
> That only reinforced my feeling that the patch below is a spec hack
> and that we ought to find a better way of preserving the restrict-ness
> across IPA optimizations.  Therefore I will keep the bug open and
> revert this patch after 4.8 is branched and stage1 opens.
>
> Meanwhile, this passes bootstrap and testsuite on x86_64-linux.  OK
> for trunk?
>
> Thanks,
>
> Martin
>
>
> 2013-02-07  Martin Jambor  <mjambor@suse.cz>
>
>         PR tree-optimization/55334
>         * ipa-cp.c (initialize_node_lattices): Disable IPA-CP through and to
>         restricted pointers to arrays.
>
> Index: src/gcc/ipa-cp.c
> ===================================================================
> --- src.orig/gcc/ipa-cp.c
> +++ src/gcc/ipa-cp.c
> @@ -730,6 +730,22 @@ initialize_node_lattices (struct cgraph_
>                  cgraph_node_name (node), node->uid,
>                  disable ? "BOTTOM" : "VARIABLE");
>      }
> +  if (!disable)
> +    for (i = 0; i < ipa_get_param_count (info) ; i++)
> +      {
> +       struct ipcp_param_lattices *plats = ipa_get_parm_lattices (info, i);
> +       tree t = TREE_TYPE (ipa_get_param(info, i));
> +
> +       if (TYPE_RESTRICT (t) && POINTER_TYPE_P (t)

You want to first test for POINTER_TYPE_P and then for TYPE_RESTRICT.

> +           && TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE)

With only testing for ARRAY_TYPE here you will still IPA-CP array
descriptors, right?  Those are RECORD_TYPE, the parameter being
DECL_BY_REFERENCE.  So I'd try instead

                && DECL_BY_REFERENCE (ipa_get_parm (info, i))

which leads me to the question - what is ipa_get_parm (info, i) returning?
Is it the lattice _value_?  Or is it the PARM_DECL of the callee (which is
the only important piece of information!)?

Thanks,
Richard.

> +         {
> +           set_lattice_to_bottom (&plats->itself);
> +           if (dump_file && (dump_flags & TDF_DETAILS)
> +               && !node->alias && !node->thunk.thunk_p)
> +             fprintf (dump_file, "Going to ignore param %i of of %s/%i.\n",
> +                      i, cgraph_node_name (node), node->uid);
> +         }
> +      }
>
>    for (ie = node->indirect_calls; ie; ie = ie->next_callee)
>      if (ie->indirect_info->polymorphic)
>
Martin Jambor - Feb. 19, 2013, 11:22 a.m.
Hi,

On Tue, Feb 19, 2013 at 10:21:32AM +0100, Richard Biener wrote:
> On Mon, Feb 18, 2013 at 9:06 PM, Martin Jambor <mjambor@suse.cz> wrote:
> > Hi,
> >
> > after much pondering about PR 55334 I came to conclusion that no nice
> > fix to the regression could be introduced in stage4.  So for the sake
> > of the SPEC 200 benchmark I decided to cripple IPA-CP on restrict
> > pointers to arrays so that the restrict-ness of the memory references
> > which need to be vectorized is not lost.
> >
> > I have tried various less aggressive tricks like only propagating the
> > one constant address that heuristics decided was profitable and not
> > all the others that came from the same calling contexts (and were
> > restrict) but then inlining came along and finished the deed with the
> > same consequences as IPA-CP had.
> >
> > That only reinforced my feeling that the patch below is a spec hack
> > and that we ought to find a better way of preserving the restrict-ness
> > across IPA optimizations.  Therefore I will keep the bug open and
> > revert this patch after 4.8 is branched and stage1 opens.
> >
> > Meanwhile, this passes bootstrap and testsuite on x86_64-linux.  OK
> > for trunk?
> >
> > Thanks,
> >
> > Martin
> >
> >
> > 2013-02-07  Martin Jambor  <mjambor@suse.cz>
> >
> >         PR tree-optimization/55334
> >         * ipa-cp.c (initialize_node_lattices): Disable IPA-CP through and to
> >         restricted pointers to arrays.
> >
> > Index: src/gcc/ipa-cp.c
> > ===================================================================
> > --- src.orig/gcc/ipa-cp.c
> > +++ src/gcc/ipa-cp.c
> > @@ -730,6 +730,22 @@ initialize_node_lattices (struct cgraph_
> >                  cgraph_node_name (node), node->uid,
> >                  disable ? "BOTTOM" : "VARIABLE");
> >      }
> > +  if (!disable)
> > +    for (i = 0; i < ipa_get_param_count (info) ; i++)
> > +      {
> > +       struct ipcp_param_lattices *plats = ipa_get_parm_lattices (info, i);
> > +       tree t = TREE_TYPE (ipa_get_param(info, i));
> > +
> > +       if (TYPE_RESTRICT (t) && POINTER_TYPE_P (t)
> 
> You want to first test for POINTER_TYPE_P and then for TYPE_RESTRICT.
> 

I see, will do.

> > +           && TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE)
> 
> With only testing for ARRAY_TYPE here you will still IPA-CP array
> descriptors, right?  Those are RECORD_TYPE, the parameter being
> DECL_BY_REFERENCE.

Possibly, why do you think that matters?  In reality, can the
descriptors ever be global variables and thus propagated?  (This is
all about the old scalar IPA-CP, it has nothing to do with the new
aggregate one.  It's just that in 4.8 heuristics decides to clone
since an otherwise unrelated patch in r190015.  In fact, aggregate
IPA-CP is not disabled by this patch).

> So I'd try instead
> 
>                 && DECL_BY_REFERENCE (ipa_get_parm (info, i))
> 

This would also disable propagation of simple scalar constants passed
by reference the Fortran way (I have just checked by looking at of PR
53787) which I specifically did not want to do.  Therefore, I'd rather
stick to my original proposal (modulo the order of pointer and
restrict tests order).

> which leads me to the question - what is ipa_get_parm (info, i) returning?
> Is it the lattice _value_?  Or is it the PARM_DECL of the callee (which is
> the only important piece of information!)?

The PARM_DECL.

Thanks,

Martin
Richard Guenther - Feb. 19, 2013, 11:40 a.m.
On Tue, Feb 19, 2013 at 12:22 PM, Martin Jambor <mjambor@suse.cz> wrote:
> Hi,
>
> On Tue, Feb 19, 2013 at 10:21:32AM +0100, Richard Biener wrote:
>> On Mon, Feb 18, 2013 at 9:06 PM, Martin Jambor <mjambor@suse.cz> wrote:
>> > Hi,
>> >
>> > after much pondering about PR 55334 I came to conclusion that no nice
>> > fix to the regression could be introduced in stage4.  So for the sake
>> > of the SPEC 200 benchmark I decided to cripple IPA-CP on restrict
>> > pointers to arrays so that the restrict-ness of the memory references
>> > which need to be vectorized is not lost.
>> >
>> > I have tried various less aggressive tricks like only propagating the
>> > one constant address that heuristics decided was profitable and not
>> > all the others that came from the same calling contexts (and were
>> > restrict) but then inlining came along and finished the deed with the
>> > same consequences as IPA-CP had.
>> >
>> > That only reinforced my feeling that the patch below is a spec hack
>> > and that we ought to find a better way of preserving the restrict-ness
>> > across IPA optimizations.  Therefore I will keep the bug open and
>> > revert this patch after 4.8 is branched and stage1 opens.
>> >
>> > Meanwhile, this passes bootstrap and testsuite on x86_64-linux.  OK
>> > for trunk?
>> >
>> > Thanks,
>> >
>> > Martin
>> >
>> >
>> > 2013-02-07  Martin Jambor  <mjambor@suse.cz>
>> >
>> >         PR tree-optimization/55334
>> >         * ipa-cp.c (initialize_node_lattices): Disable IPA-CP through and to
>> >         restricted pointers to arrays.
>> >
>> > Index: src/gcc/ipa-cp.c
>> > ===================================================================
>> > --- src.orig/gcc/ipa-cp.c
>> > +++ src/gcc/ipa-cp.c
>> > @@ -730,6 +730,22 @@ initialize_node_lattices (struct cgraph_
>> >                  cgraph_node_name (node), node->uid,
>> >                  disable ? "BOTTOM" : "VARIABLE");
>> >      }
>> > +  if (!disable)
>> > +    for (i = 0; i < ipa_get_param_count (info) ; i++)
>> > +      {
>> > +       struct ipcp_param_lattices *plats = ipa_get_parm_lattices (info, i);
>> > +       tree t = TREE_TYPE (ipa_get_param(info, i));
>> > +
>> > +       if (TYPE_RESTRICT (t) && POINTER_TYPE_P (t)
>>
>> You want to first test for POINTER_TYPE_P and then for TYPE_RESTRICT.
>>
>
> I see, will do.
>
>> > +           && TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE)
>>
>> With only testing for ARRAY_TYPE here you will still IPA-CP array
>> descriptors, right?  Those are RECORD_TYPE, the parameter being
>> DECL_BY_REFERENCE.
>
> Possibly, why do you think that matters?  In reality, can the
> descriptors ever be global variables and thus propagated?  (This is
> all about the old scalar IPA-CP, it has nothing to do with the new
> aggregate one.  It's just that in 4.8 heuristics decides to clone
> since an otherwise unrelated patch in r190015.  In fact, aggregate
> IPA-CP is not disabled by this patch).

I'm not sure about global array descriptors but I seem to remember seeing
those.

>> So I'd try instead
>>
>>                 && DECL_BY_REFERENCE (ipa_get_parm (info, i))
>>
>
> This would also disable propagation of simple scalar constants passed
> by reference the Fortran way (I have just checked by looking at of PR
> 53787) which I specifically did not want to do.  Therefore, I'd rather
> stick to my original proposal (modulo the order of pointer and
> restrict tests order).

Ok, that makes sense.

>> which leads me to the question - what is ipa_get_parm (info, i) returning?
>> Is it the lattice _value_?  Or is it the PARM_DECL of the callee (which is
>> the only important piece of information!)?
>
> The PARM_DECL.

Fine.

The patch is ok then, with the restrict / pointer test re-ordered.

Thanks,
Richard.

> Thanks,
>
> Martin

Patch

Index: src/gcc/ipa-cp.c
===================================================================
--- src.orig/gcc/ipa-cp.c
+++ src/gcc/ipa-cp.c
@@ -730,6 +730,22 @@  initialize_node_lattices (struct cgraph_
 		 cgraph_node_name (node), node->uid,
 		 disable ? "BOTTOM" : "VARIABLE");
     }
+  if (!disable)
+    for (i = 0; i < ipa_get_param_count (info) ; i++)
+      {
+	struct ipcp_param_lattices *plats = ipa_get_parm_lattices (info, i);
+	tree t = TREE_TYPE (ipa_get_param(info, i));
+
+	if (TYPE_RESTRICT (t) && POINTER_TYPE_P (t)
+	    && TREE_CODE (TREE_TYPE (t)) == ARRAY_TYPE)
+	  {
+	    set_lattice_to_bottom (&plats->itself);
+	    if (dump_file && (dump_flags & TDF_DETAILS)
+		&& !node->alias && !node->thunk.thunk_p)
+	      fprintf (dump_file, "Going to ignore param %i of of %s/%i.\n",
+		       i, cgraph_node_name (node), node->uid);
+	  }
+      }
 
   for (ie = node->indirect_calls; ie; ie = ie->next_callee)
     if (ie->indirect_info->polymorphic)