Re: [PATCH] Fix PR77937

[Bill Schmidt]

> 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

[Richard Biener]

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

[Bill Schmidt]

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

[Richard Biener]

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.
>
>

[PATCH] Fix PR77937

[Bill Schmidt]

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  

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

[Markus Trippelsdorf]

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

[PATCH] Fix PR77937

[Bill Schmidt]

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.

Thanks,
Bill


2016-10-12  Bill Schmidt  

PR tree-optimization/77937
* gimple-ssa-strength-reduction.c (analyze_increments): Use
POINTER_TYPE_P on the candidate type to determine whether
candidates in this chain require pointer arithmetic.


Index: gcc/gimple-ssa-strength-reduction.c
===
--- gcc/gimple-ssa-strength-reduction.c (revision 240946)
+++ gcc/gimple-ssa-strength-reduction.c (working copy)
@@ -2816,8 +2816,7 @@ analyze_increments (slsr_cand_t first_dep, machine
   else if (incr == 0
   || incr == 1
   || (incr == -1
-  && (gimple_assign_rhs_code (first_dep->cand_stmt)
-  != POINTER_PLUS_EXPR)))
+  && !POINTER_TYPE_P (first_dep->cand_type)))
incr_vec[i].cost = COST_NEUTRAL;
   
   /* FORNOW: If we need to add an initializer, give up if a cast from