| Message ID | 20130218200623.GB19120@virgil.suse |
|---|---|
| State | New |
| Headers | show |
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) >
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
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
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)