Re: [PATCH] Fix PR77916

[Bill Schmidt]

On Tue, 2016-10-18 at 15:30 +0200, Markus Trippelsdorf wrote:

> I you wish I can send you a tarball with the preprocessed *.i files from
> ffmpeg, so that you can use a stage1 cross on them.
> 

That would be very helpful, thanks!

Bill

Re: [PATCH] Fix PR77916

[Bill Schmidt]

Hi,

The previous solution for PR77916 was inadequately tested, for which I
sincerely apologize.  I've reinstated the stopgap fix previously
reverted, as follows.

Thanks for your patience,
Bill


2016-10-18  Bill Schmidt  

PR tree-optimization/77916
* gimple-ssa-strength-reduction.c (analyze_increments): Reinstate
stopgap fix, as pointers with -1 increment are still broken.

Index: gcc/gimple-ssa-strength-reduction.c
===
--- gcc/gimple-ssa-strength-reduction.c (revision 241302)
+++ gcc/gimple-ssa-strength-reduction.c (working copy)
@@ -2825,6 +2825,10 @@ analyze_increments (slsr_cand_t first_dep, machine
   && !POINTER_TYPE_P (first_dep->cand_type)))
incr_vec[i].cost = COST_NEUTRAL;
 
+  /* FIXME: Still having trouble with pointers with a -1 increment.  */
+  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.
 This could eventually be handled better by expressly retaining the

Re: [PATCH] Fix PR77916

[Markus Trippelsdorf]

On 2016.10.18 at 08:15 -0500, Bill Schmidt wrote:
> On Tue, 2016-10-18 at 05:13 +0200, Markus Trippelsdorf wrote:
> > On 2016.10.17 at 17:23 -0500, Bill Schmidt wrote:
> > > Hi,
> > > 
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77916 identifies a situation
> > > where SLSR will ICE when exposed to a cast from integer to pointer.  This
> > > is because we try to convert a PLUS_EXPR with an addend of -1 * S into a
> > > MINUS_EXPR with a subtrahend of S, but the base operand is unexpectedly
> > > of pointer type.  This patch recognizes when pointer arithmetic is taking
> > > place and ensures that we use a POINTER_PLUS_EXPR at all such times.  In
> > > the case of the PR, this occurs in the logic where the stride S is a known
> > > constant value, but the same problem could occur when it is an SSA_NAME
> > > without known value.  Both possibilities are handled here.
> > > 
> > > Fixing the code to ensure that the unknown stride case always uses an 
> > > initializer for a negative increment allows us to remove the stopgap fix
> > > added for PR77937 as well.
> > > 
> > > Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no
> > > regressions, committed.
> > 
> > Perhaps you should consider building ffmpeg with -O3 -march=amdfam10 on
> > X86_64 before committing these patches, because you broke it for the
> > third time in the last couple of days.
> 
> Sorry, sorry.  I did intend to build another stage1 cross and try this,
> but it just slipped my mind.  Looks like I'll need to put the stopgap
> fix back in.  I'll do that shortly.
> 
> Meantime, -fno-slsr is a workaround.  If I have trouble building ffmpeg
> with a cross, I'll check with you on testing a future patch.

I you wish I can send you a tarball with the preprocessed *.i files from
ffmpeg, so that you can use a stage1 cross on them.

-- 
Markus

Re: [PATCH] Fix PR77916

[Bill Schmidt]

On Tue, 2016-10-18 at 05:13 +0200, Markus Trippelsdorf wrote:
> On 2016.10.17 at 17:23 -0500, Bill Schmidt wrote:
> > Hi,
> > 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77916 identifies a situation
> > where SLSR will ICE when exposed to a cast from integer to pointer.  This
> > is because we try to convert a PLUS_EXPR with an addend of -1 * S into a
> > MINUS_EXPR with a subtrahend of S, but the base operand is unexpectedly
> > of pointer type.  This patch recognizes when pointer arithmetic is taking
> > place and ensures that we use a POINTER_PLUS_EXPR at all such times.  In
> > the case of the PR, this occurs in the logic where the stride S is a known
> > constant value, but the same problem could occur when it is an SSA_NAME
> > without known value.  Both possibilities are handled here.
> > 
> > Fixing the code to ensure that the unknown stride case always uses an 
> > initializer for a negative increment allows us to remove the stopgap fix
> > added for PR77937 as well.
> > 
> > Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no
> > regressions, committed.
> 
> Perhaps you should consider building ffmpeg with -O3 -march=amdfam10 on
> X86_64 before committing these patches, because you broke it for the
> third time in the last couple of days.

Sorry, sorry.  I did intend to build another stage1 cross and try this,
but it just slipped my mind.  Looks like I'll need to put the stopgap
fix back in.  I'll do that shortly.

Meantime, -fno-slsr is a workaround.  If I have trouble building ffmpeg
with a cross, I'll check with you on testing a future patch.

Bill

> 
> markus@x4 ffmpeg % cat h264dsp.i
> extern int fn2(int);
> extern int fn3(int);
> int a, b, c;
> void fn1(long p1) {
>   char *d;
>   for (;; d += p1) {
> d[0] = fn2(1 >> c >> 1);
> fn2(c >> a);
> d[1] = fn3(d[1]) >> 1;
> d[6] = fn3(d[6] * b + 1) >> 1;
> d[7] = fn3(d[7] * b + 1) >> 1;
> d[8] = fn3(d[8] * b + 1) >> 1;
>   }
> }
> 
> markus@x4 ffmpeg % gcc -O3 -march=amdfam10 -c h264dsp.i
> h264dsp.i: In function ‘fn1’:
> h264dsp.i:4:6: internal compiler error: in replace_one_candidate, at 
> gimple-ssa-strength-reduction.c:3375
>  void fn1(long p1) {
>   ^~~
> 0x12773a9 replace_one_candidate
> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3375
> 0x127af77 replace_profitable_candidates
> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3486
> 0x127aeeb replace_profitable_candidates
> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3495
> 0x127f3ee analyze_candidates_and_replace
> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3574
> 0x127f3ee execute
> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3648
> 
> 
> 
> 

Re: [PATCH] Fix PR77916

[Markus Trippelsdorf]

On 2016.10.18 at 11:19 +0200, Christophe Lyon wrote:
> On 18 October 2016 at 05:18, Markus Trippelsdorf  
> wrote:
> > On 2016.10.18 at 05:13 +0200, Markus Trippelsdorf wrote:
> >> On 2016.10.17 at 17:23 -0500, Bill Schmidt wrote:
> >> > Hi,
> >> >
> >> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77916 identifies a situation
> >> > where SLSR will ICE when exposed to a cast from integer to pointer.  This
> >> > is because we try to convert a PLUS_EXPR with an addend of -1 * S into a
> >> > MINUS_EXPR with a subtrahend of S, but the base operand is unexpectedly
> >> > of pointer type.  This patch recognizes when pointer arithmetic is taking
> >> > place and ensures that we use a POINTER_PLUS_EXPR at all such times.  In
> >> > the case of the PR, this occurs in the logic where the stride S is a 
> >> > known
> >> > constant value, but the same problem could occur when it is an SSA_NAME
> >> > without known value.  Both possibilities are handled here.
> >> >
> >> > Fixing the code to ensure that the unknown stride case always uses an
> >> > initializer for a negative increment allows us to remove the stopgap fix
> >> > added for PR77937 as well.
> >> >
> >> > Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no
> >> > regressions, committed.
> >>
> >> Perhaps you should consider building ffmpeg with -O3 -march=amdfam10 on
> >> X86_64 before committing these patches, because you broke it for the
> >> third time in the last couple of days.
> >>
> >> markus@x4 ffmpeg % cat h264dsp.i
> >> extern int fn2(int);
> >> extern int fn3(int);
> >> int a, b, c;
> >> void fn1(long p1) {
> >>   char *d;
> >>   for (;; d += p1) {
> >> d[0] = fn2(1 >> c >> 1);
> >> fn2(c >> a);
> >> d[1] = fn3(d[1]) >> 1;
> >> d[6] = fn3(d[6] * b + 1) >> 1;
> >> d[7] = fn3(d[7] * b + 1) >> 1;
> >> d[8] = fn3(d[8] * b + 1) >> 1;
> >>   }
> >> }
> >>
> >> markus@x4 ffmpeg % gcc -O3 -march=amdfam10 -c h264dsp.i
> >> h264dsp.i: In function ‘fn1’:
> >> h264dsp.i:4:6: internal compiler error: in replace_one_candidate, at 
> >> gimple-ssa-strength-reduction.c:3375
> >>  void fn1(long p1) {
> >>   ^~~
> >> 0x12773a9 replace_one_candidate
> >> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3375
> >> 0x127af77 replace_profitable_candidates
> >> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3486
> >> 0x127aeeb replace_profitable_candidates
> >> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3495
> >> 0x127f3ee analyze_candidates_and_replace
> >> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3574
> >> 0x127f3ee execute
> >> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3648
> >
> > Just figured out that this testcase is identical to:
> > gcc/testsuite/gcc.dg/torture/pr77937-2.c
> >
> > So please run the testsuite on X86_64 in the future.
> >
> 
> 
> I'm not sure whether Markus means that pr77937-2 fails since this
> commit?
> 
> I'm seeing ICEs on pr77937-2 on some arm targets:
> http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/241285/report-build-info.html
> 
> but I'm not 100% sure it was caused by this patch (the regression
> happened between r241273 and r241285), I haven't looked in details
> yet.

Yes, this is caused by this patch.

-- 
Markus

Re: [PATCH] Fix PR77916

[Christophe Lyon]

On 18 October 2016 at 05:18, Markus Trippelsdorf  wrote:
> On 2016.10.18 at 05:13 +0200, Markus Trippelsdorf wrote:
>> On 2016.10.17 at 17:23 -0500, Bill Schmidt wrote:
>> > Hi,
>> >
>> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77916 identifies a situation
>> > where SLSR will ICE when exposed to a cast from integer to pointer.  This
>> > is because we try to convert a PLUS_EXPR with an addend of -1 * S into a
>> > MINUS_EXPR with a subtrahend of S, but the base operand is unexpectedly
>> > of pointer type.  This patch recognizes when pointer arithmetic is taking
>> > place and ensures that we use a POINTER_PLUS_EXPR at all such times.  In
>> > the case of the PR, this occurs in the logic where the stride S is a known
>> > constant value, but the same problem could occur when it is an SSA_NAME
>> > without known value.  Both possibilities are handled here.
>> >
>> > Fixing the code to ensure that the unknown stride case always uses an
>> > initializer for a negative increment allows us to remove the stopgap fix
>> > added for PR77937 as well.
>> >
>> > Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no
>> > regressions, committed.
>>
>> Perhaps you should consider building ffmpeg with -O3 -march=amdfam10 on
>> X86_64 before committing these patches, because you broke it for the
>> third time in the last couple of days.
>>
>> markus@x4 ffmpeg % cat h264dsp.i
>> extern int fn2(int);
>> extern int fn3(int);
>> int a, b, c;
>> void fn1(long p1) {
>>   char *d;
>>   for (;; d += p1) {
>> d[0] = fn2(1 >> c >> 1);
>> fn2(c >> a);
>> d[1] = fn3(d[1]) >> 1;
>> d[6] = fn3(d[6] * b + 1) >> 1;
>> d[7] = fn3(d[7] * b + 1) >> 1;
>> d[8] = fn3(d[8] * b + 1) >> 1;
>>   }
>> }
>>
>> markus@x4 ffmpeg % gcc -O3 -march=amdfam10 -c h264dsp.i
>> h264dsp.i: In function ‘fn1’:
>> h264dsp.i:4:6: internal compiler error: in replace_one_candidate, at 
>> gimple-ssa-strength-reduction.c:3375
>>  void fn1(long p1) {
>>   ^~~
>> 0x12773a9 replace_one_candidate
>> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3375
>> 0x127af77 replace_profitable_candidates
>> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3486
>> 0x127aeeb replace_profitable_candidates
>> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3495
>> 0x127f3ee analyze_candidates_and_replace
>> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3574
>> 0x127f3ee execute
>> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3648
>
> Just figured out that this testcase is identical to:
> gcc/testsuite/gcc.dg/torture/pr77937-2.c
>
> So please run the testsuite on X86_64 in the future.
>


I'm not sure whether Markus means that pr77937-2 fails since this commit?

I'm seeing ICEs on pr77937-2 on some arm targets:
http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/241285/report-build-info.html

but I'm not 100% sure it was caused by this patch (the regression
happened between
r241273 and r241285), I haven't looked in details yet.

Christophe

> --
> Markus

Re: [PATCH] Fix PR77916

[Markus Trippelsdorf]

On 2016.10.18 at 05:13 +0200, Markus Trippelsdorf wrote:
> On 2016.10.17 at 17:23 -0500, Bill Schmidt wrote:
> > Hi,
> > 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77916 identifies a situation
> > where SLSR will ICE when exposed to a cast from integer to pointer.  This
> > is because we try to convert a PLUS_EXPR with an addend of -1 * S into a
> > MINUS_EXPR with a subtrahend of S, but the base operand is unexpectedly
> > of pointer type.  This patch recognizes when pointer arithmetic is taking
> > place and ensures that we use a POINTER_PLUS_EXPR at all such times.  In
> > the case of the PR, this occurs in the logic where the stride S is a known
> > constant value, but the same problem could occur when it is an SSA_NAME
> > without known value.  Both possibilities are handled here.
> > 
> > Fixing the code to ensure that the unknown stride case always uses an 
> > initializer for a negative increment allows us to remove the stopgap fix
> > added for PR77937 as well.
> > 
> > Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no
> > regressions, committed.
> 
> Perhaps you should consider building ffmpeg with -O3 -march=amdfam10 on
> X86_64 before committing these patches, because you broke it for the
> third time in the last couple of days.
> 
> markus@x4 ffmpeg % cat h264dsp.i
> extern int fn2(int);
> extern int fn3(int);
> int a, b, c;
> void fn1(long p1) {
>   char *d;
>   for (;; d += p1) {
> d[0] = fn2(1 >> c >> 1);
> fn2(c >> a);
> d[1] = fn3(d[1]) >> 1;
> d[6] = fn3(d[6] * b + 1) >> 1;
> d[7] = fn3(d[7] * b + 1) >> 1;
> d[8] = fn3(d[8] * b + 1) >> 1;
>   }
> }
> 
> markus@x4 ffmpeg % gcc -O3 -march=amdfam10 -c h264dsp.i
> h264dsp.i: In function ‘fn1’:
> h264dsp.i:4:6: internal compiler error: in replace_one_candidate, at 
> gimple-ssa-strength-reduction.c:3375
>  void fn1(long p1) {
>   ^~~
> 0x12773a9 replace_one_candidate
> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3375
> 0x127af77 replace_profitable_candidates
> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3486
> 0x127aeeb replace_profitable_candidates
> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3495
> 0x127f3ee analyze_candidates_and_replace
> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3574
> 0x127f3ee execute
> ../../gcc/gcc/gimple-ssa-strength-reduction.c:3648

Just figured out that this testcase is identical to:
gcc/testsuite/gcc.dg/torture/pr77937-2.c 

So please run the testsuite on X86_64 in the future.

-- 
Markus

Re: [PATCH] Fix PR77916

[Markus Trippelsdorf]

On 2016.10.17 at 17:23 -0500, Bill Schmidt wrote:
> Hi,
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=77916 identifies a situation
> where SLSR will ICE when exposed to a cast from integer to pointer.  This
> is because we try to convert a PLUS_EXPR with an addend of -1 * S into a
> MINUS_EXPR with a subtrahend of S, but the base operand is unexpectedly
> of pointer type.  This patch recognizes when pointer arithmetic is taking
> place and ensures that we use a POINTER_PLUS_EXPR at all such times.  In
> the case of the PR, this occurs in the logic where the stride S is a known
> constant value, but the same problem could occur when it is an SSA_NAME
> without known value.  Both possibilities are handled here.
> 
> Fixing the code to ensure that the unknown stride case always uses an 
> initializer for a negative increment allows us to remove the stopgap fix
> added for PR77937 as well.
> 
> Bootstrapped and tested on powerpc64le-unknown-linux-gnu with no
> regressions, committed.

Perhaps you should consider building ffmpeg with -O3 -march=amdfam10 on
X86_64 before committing these patches, because you broke it for the
third time in the last couple of days.

markus@x4 ffmpeg % cat h264dsp.i
extern int fn2(int);
extern int fn3(int);
int a, b, c;
void fn1(long p1) {
  char *d;
  for (;; d += p1) {
d[0] = fn2(1 >> c >> 1);
fn2(c >> a);
d[1] = fn3(d[1]) >> 1;
d[6] = fn3(d[6] * b + 1) >> 1;
d[7] = fn3(d[7] * b + 1) >> 1;
d[8] = fn3(d[8] * b + 1) >> 1;
  }
}

markus@x4 ffmpeg % gcc -O3 -march=amdfam10 -c h264dsp.i
h264dsp.i: In function ‘fn1’:
h264dsp.i:4:6: internal compiler error: in replace_one_candidate, at 
gimple-ssa-strength-reduction.c:3375
 void fn1(long p1) {
  ^~~
0x12773a9 replace_one_candidate
../../gcc/gcc/gimple-ssa-strength-reduction.c:3375
0x127af77 replace_profitable_candidates
../../gcc/gcc/gimple-ssa-strength-reduction.c:3486
0x127aeeb replace_profitable_candidates
../../gcc/gcc/gimple-ssa-strength-reduction.c:3495
0x127f3ee analyze_candidates_and_replace
../../gcc/gcc/gimple-ssa-strength-reduction.c:3574
0x127f3ee execute
../../gcc/gcc/gimple-ssa-strength-reduction.c:3648




-- 
Markus