diff mbox

Fix PR77937

Message ID 1476373127.5711.3.camel@oc8801110288.ibm.com
State New
Headers show

Commit Message

Bill Schmidt Oct. 13, 2016, 3:38 p.m. UTC 
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.

Thanks,
Bill


2016-10-13  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	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.

Comments

Richard Biener Oct. 14, 2016, 9:19 a.m. UTC | #1 
On Thu, Oct 13, 2016 at 5:38 PM, Bill Schmidt
<wschmidt@linux.vnet.ibm.com> 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  <wschmidt@linux.vnet.ibm.com>
>
>         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.
>
>
Bill Schmidt Oct. 14, 2016, 1:51 p.m. UTC | #2 
Hi Richard,

> On Oct 14, 2016, at 4:19 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Thu, Oct 13, 2016 at 5:38 PM, Bill Schmidt
> <wschmidt@linux.vnet.ibm.com> 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  <wschmidt@linux.vnet.ibm.com>
>> 
>>        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.
>> 
>> 
>
Richard Biener Oct. 17, 2016, 8:27 a.m. UTC | #3 
On Fri, Oct 14, 2016 at 3:51 PM, Bill Schmidt
<wschmidt@linux.vnet.ibm.com> wrote:
> Hi Richard,
>
>> On Oct 14, 2016, at 4:19 AM, Richard Biener <richard.guenther@gmail.com> wrote:
>>
>> On Thu, Oct 13, 2016 at 5:38 PM, Bill Schmidt
>> <wschmidt@linux.vnet.ibm.com> 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  <wschmidt@linux.vnet.ibm.com>
>>>
>>>        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.
>>>
>>>
>>
>
Bill Schmidt Oct. 17, 2016, 1:08 p.m. UTC | #4 
> On Oct 17, 2016, at 3:27 AM, Richard Biener <richard.guenther@gmail.com> wrote:
> 
> On Fri, Oct 14, 2016 at 3:51 PM, Bill Schmidt
> <wschmidt@linux.vnet.ibm.com> wrote:
>> Hi Richard,
>> 
>>> On Oct 14, 2016, at 4:19 AM, Richard Biener <richard.guenther@gmail.com> wrote:
>>> 
>>> On Thu, Oct 13, 2016 at 5:38 PM, Bill Schmidt
>>> <wschmidt@linux.vnet.ibm.com> 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  <wschmidt@linux.vnet.ibm.com>
>>>> 
>>>>       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.
diff mbox

Patch

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.