Re: [PATCH] Fix PR77937
> On Oct 17, 2016, at 3:27 AM, Richard Biener > wrote: > > On Fri, Oct 14, 2016 at 3:51 PM, Bill Schmidt > wrote: >> Hi Richard, >> >>> On Oct 14, 2016, at 4:19 AM, Richard Biener >>> wrote: >>> >>> On Thu, Oct 13, 2016 at 5:38 PM, Bill Schmidt >>> wrote: The previous patch for https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77937 is necessary, but not sufficient in all cases. It allows -1 to be used with a pointer increment, which we really do not want given that this is generally not profitable. Disable this case for now. We can add logic later to estimate the cost for the rare case where it can be useful. Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no regressions, committed. >>> >>> Huh, I wonder what is special about -1 here. Do we handle -2? >> >> We do, subject to a little more stringent cost modeling later on, because it >> requires introducing a multiply by the increment. We have some special >> case code for -1 that introduces a MINUS_EXPR, but that breaks for >> pointer arithmetic. > > Ah, ok. Fine then. > >> I am working on a better fix for this as part of the work for PR77916, which >> exposes a related problem elsewhere in the code. The current fix is a >> stopgap until I can get that work completed. For -1 we prefer a negate >> over a multiply when we have pointer types and can't use minus, and need >> to properly model that in the cost calculation. > > Note that RTL expansion will turn this into a minus again so I dont' think you > need any cost adjustment here. It's just that GIMPLE doesnt' have a > POINTER_MINUS_EXPR... > (RTL just has plus and minus, nothing special for pointers). I came to the same conclusion over the weekend. So I think the real fix will be pretty simple now. I keep getting pulled off to other things, but should have something for review in the next couple of days. Thanks! Bill > > Richard. > >> Bill >> >>> >>> Richard. >>> Thanks, Bill 2016-10-13 Bill Schmidt PR tree-optimization/77937 * gimple-ssa-strength-reduction.c (analyze_increments): Set cost to infinite when we have a pointer with an increment of -1. Index: gcc/gimple-ssa-strength-reduction.c === --- gcc/gimple-ssa-strength-reduction.c (revision 241120) +++ gcc/gimple-ssa-strength-reduction.c (working copy) @@ -2818,6 +2818,11 @@ analyze_increments (slsr_cand_t first_dep, machine || (incr == -1 && !POINTER_TYPE_P (first_dep->cand_type))) incr_vec[i].cost = COST_NEUTRAL; + + /* FIXME: We don't handle pointers with a -1 increment yet. + They are usually unprofitable anyway. */ + else if (incr == -1 && POINTER_TYPE_P (first_dep->cand_type)) + incr_vec[i].cost = COST_INFINITE; /* FORNOW: If we need to add an initializer, give up if a cast from the candidate's type to its stride's type can lose precision.
Re: [PATCH] Fix PR77937
On Fri, Oct 14, 2016 at 3:51 PM, Bill Schmidt wrote: > Hi Richard, > >> On Oct 14, 2016, at 4:19 AM, Richard Biener >> wrote: >> >> On Thu, Oct 13, 2016 at 5:38 PM, Bill Schmidt >> wrote: >>> The previous patch for >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77937 is necessary, but not >>> sufficient in all cases. It allows -1 to be used with a pointer >>> increment, which we really do not want given that this is generally not >>> profitable. Disable this case for now. We can add logic later to >>> estimate the cost for the rare case where it can be useful. >>> >>> Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no >>> regressions, committed. >> >> Huh, I wonder what is special about -1 here. Do we handle -2? > > We do, subject to a little more stringent cost modeling later on, because it > requires introducing a multiply by the increment. We have some special > case code for -1 that introduces a MINUS_EXPR, but that breaks for > pointer arithmetic. Ah, ok. Fine then. > I am working on a better fix for this as part of the work for PR77916, which > exposes a related problem elsewhere in the code. The current fix is a > stopgap until I can get that work completed. For -1 we prefer a negate > over a multiply when we have pointer types and can't use minus, and need > to properly model that in the cost calculation. Note that RTL expansion will turn this into a minus again so I dont' think you need any cost adjustment here. It's just that GIMPLE doesnt' have a POINTER_MINUS_EXPR... (RTL just has plus and minus, nothing special for pointers). Richard. > Bill > >> >> Richard. >> >>> Thanks, >>> Bill >>> >>> >>> 2016-10-13 Bill Schmidt >>> >>>PR tree-optimization/77937 >>>* gimple-ssa-strength-reduction.c (analyze_increments): Set cost >>>to infinite when we have a pointer with an increment of -1. >>> >>> >>> Index: gcc/gimple-ssa-strength-reduction.c >>> === >>> --- gcc/gimple-ssa-strength-reduction.c (revision 241120) >>> +++ gcc/gimple-ssa-strength-reduction.c (working copy) >>> @@ -2818,6 +2818,11 @@ analyze_increments (slsr_cand_t first_dep, machine >>> || (incr == -1 >>> && !POINTER_TYPE_P (first_dep->cand_type))) >>>incr_vec[i].cost = COST_NEUTRAL; >>> + >>> + /* FIXME: We don't handle pointers with a -1 increment yet. >>> + They are usually unprofitable anyway. */ >>> + else if (incr == -1 && POINTER_TYPE_P (first_dep->cand_type)) >>> + incr_vec[i].cost = COST_INFINITE; >>> >>> /* FORNOW: If we need to add an initializer, give up if a cast from >>> the candidate's type to its stride's type can lose precision. >>> >>> >> >
Re: [PATCH] Fix PR77937
Hi Richard, > On Oct 14, 2016, at 4:19 AM, Richard Biener > wrote: > > On Thu, Oct 13, 2016 at 5:38 PM, Bill Schmidt > wrote: >> The previous patch for >> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77937 is necessary, but not >> sufficient in all cases. It allows -1 to be used with a pointer >> increment, which we really do not want given that this is generally not >> profitable. Disable this case for now. We can add logic later to >> estimate the cost for the rare case where it can be useful. >> >> Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no >> regressions, committed. > > Huh, I wonder what is special about -1 here. Do we handle -2? We do, subject to a little more stringent cost modeling later on, because it requires introducing a multiply by the increment. We have some special case code for -1 that introduces a MINUS_EXPR, but that breaks for pointer arithmetic. I am working on a better fix for this as part of the work for PR77916, which exposes a related problem elsewhere in the code. The current fix is a stopgap until I can get that work completed. For -1 we prefer a negate over a multiply when we have pointer types and can't use minus, and need to properly model that in the cost calculation. Bill > > Richard. > >> Thanks, >> Bill >> >> >> 2016-10-13 Bill Schmidt >> >>PR tree-optimization/77937 >>* gimple-ssa-strength-reduction.c (analyze_increments): Set cost >>to infinite when we have a pointer with an increment of -1. >> >> >> Index: gcc/gimple-ssa-strength-reduction.c >> === >> --- gcc/gimple-ssa-strength-reduction.c (revision 241120) >> +++ gcc/gimple-ssa-strength-reduction.c (working copy) >> @@ -2818,6 +2818,11 @@ analyze_increments (slsr_cand_t first_dep, machine >> || (incr == -1 >> && !POINTER_TYPE_P (first_dep->cand_type))) >>incr_vec[i].cost = COST_NEUTRAL; >> + >> + /* FIXME: We don't handle pointers with a -1 increment yet. >> + They are usually unprofitable anyway. */ >> + else if (incr == -1 && POINTER_TYPE_P (first_dep->cand_type)) >> + incr_vec[i].cost = COST_INFINITE; >> >> /* FORNOW: If we need to add an initializer, give up if a cast from >> the candidate's type to its stride's type can lose precision. >> >> >
Re: [PATCH] Fix PR77937
On Thu, Oct 13, 2016 at 5:38 PM, Bill Schmidt wrote: > The previous patch for > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77937 is necessary, but not > sufficient in all cases. It allows -1 to be used with a pointer > increment, which we really do not want given that this is generally not > profitable. Disable this case for now. We can add logic later to > estimate the cost for the rare case where it can be useful. > > Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no > regressions, committed. Huh, I wonder what is special about -1 here. Do we handle -2? Richard. > Thanks, > Bill > > > 2016-10-13 Bill Schmidt > > PR tree-optimization/77937 > * gimple-ssa-strength-reduction.c (analyze_increments): Set cost > to infinite when we have a pointer with an increment of -1. > > > Index: gcc/gimple-ssa-strength-reduction.c > === > --- gcc/gimple-ssa-strength-reduction.c (revision 241120) > +++ gcc/gimple-ssa-strength-reduction.c (working copy) > @@ -2818,6 +2818,11 @@ analyze_increments (slsr_cand_t first_dep, machine >|| (incr == -1 >&& !POINTER_TYPE_P (first_dep->cand_type))) > incr_vec[i].cost = COST_NEUTRAL; > + > + /* FIXME: We don't handle pointers with a -1 increment yet. > + They are usually unprofitable anyway. */ > + else if (incr == -1 && POINTER_TYPE_P (first_dep->cand_type)) > + incr_vec[i].cost = COST_INFINITE; > >/* FORNOW: If we need to add an initializer, give up if a cast from > the candidate's type to its stride's type can lose precision. > >
Re: [PATCH] Fix PR77937
On 2016.10.12 at 20:14 -0500, Bill Schmidt wrote: > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77937 reports an ICE in SLSR > where a POINTER_PLUS_EXPR occurs with a candidate increment of -1. This > is supposed to be prevented by code in analyze_increments, since replacement > of such a candidate is not guaranteed to be profitable. The test for this > was inadequate. This patch replaces it with the correct check, determining > whether the type of the candidate is a pointer type. > > Bootstrapped and tested on powerpc64le-unknown-linux-gnu, committed. Please commit the small testcase from the bug, too. Thanks. -- Markus