[PATCH] Fix PR79201 (half-way)

[PATCH] Fix PR79201 (half-way)

[Richard Biener]

One issue in PR79201 is that we don't sink pure/const calls which is
what the following simple patch fixes.

Bootstrap and regtest running on x86_64-unknown-linux-gnu.

Richard.

2017-04-24  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/79201
	* tree-ssa-sink.c (statement_sink_location): Handle calls.

	* gcc.dg/tree-ssa/ssa-sink-16.c: New testcase.

Index: gcc/tree-ssa-sink.c
===================================================================
*** gcc/tree-ssa-sink.c	(revision 247091)
--- gcc/tree-ssa-sink.c	(working copy)
*************** statement_sink_location (gimple *stmt, b
*** 256,263 ****
  
    *zero_uses_p = false;
  
!   /* We only can sink assignments.  */
!   if (!is_gimple_assign (stmt))
      return false;
  
    /* We only can sink stmts with a single definition.  */
--- 257,268 ----
  
    *zero_uses_p = false;
  
!   /* We only can sink assignments and non-looping const/pure calls.  */
!   int cf;
!   if (!is_gimple_assign (stmt)
!       && (!is_gimple_call (stmt)
! 	  || !((cf = gimple_call_flags (stmt)) & (ECF_CONST|ECF_PURE))
! 	  || (cf & ECF_LOOPING_CONST_OR_PURE)))
      return false;
  
    /* We only can sink stmts with a single definition.  */
Index: gcc/testsuite/gcc.dg/tree-ssa/ssa-sink-16.c
===================================================================
*** gcc/testsuite/gcc.dg/tree-ssa/ssa-sink-16.c	(nonexistent)
--- gcc/testsuite/gcc.dg/tree-ssa/ssa-sink-16.c	(working copy)
***************
*** 0 ****
--- 1,14 ----
+ /* { dg-do compile } */
+ /* Note PRE rotates the loop and blocks the sinking opportunity.  */
+ /* { dg-options "-O2 -fno-tree-pre -fdump-tree-sink -fdump-tree-optimized" } */
+ 
+ int f(int n)
+ {
+   int i,j=0;
+   for (i = 0; i < 31; i++)
+     j = __builtin_ffs(i);
+   return j;
+ }
+ 
+ /* { dg-final { scan-tree-dump "Sinking j_. = __builtin_ffs" "sink" } } */
+ /* { dg-final { scan-tree-dump "return 2;" "optimized" } } */

Re: [PATCH] Fix PR79201 (half-way)

[Richard Biener]

On Mon, 24 Apr 2017, Richard Biener wrote:

> 
> One issue in PR79201 is that we don't sink pure/const calls which is
> what the following simple patch fixes.
> 
> Bootstrap and regtest running on x86_64-unknown-linux-gnu.

Needed some gimple_assign_lhs -> gimple_get_lhs adjustments and
adjustment of gcc.target/i386/pr22152.c where we now sink the
assignment out of the pointless loop.  Not sure what the original
bug was about (well, reg allocation) so I simply disabled sinking
for it.

Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.

Richard.

2017-04-25  Richard Biener  <rguenther@suse.de>

	PR tree-optimization/79201
	* tree-ssa-sink.c (statement_sink_location): Handle calls.

	* gcc.dg/tree-ssa/ssa-sink-16.c: New testcase.
	* gcc.target/i386/pr22152.c: Disable sinking.

Index: gcc/tree-ssa-sink.c
===================================================================
*** gcc/tree-ssa-sink.c	(revision 247092)
--- gcc/tree-ssa-sink.c	(working copy)
*************** statement_sink_location (gimple *stmt, b
*** 256,263 ****
  
    *zero_uses_p = false;
  
!   /* We only can sink assignments.  */
!   if (!is_gimple_assign (stmt))
      return false;
  
    /* We only can sink stmts with a single definition.  */
--- 257,268 ----
  
    *zero_uses_p = false;
  
!   /* We only can sink assignments and non-looping const/pure calls.  */
!   int cf;
!   if (!is_gimple_assign (stmt)
!       && (!is_gimple_call (stmt)
! 	  || !((cf = gimple_call_flags (stmt)) & (ECF_CONST|ECF_PURE))
! 	  || (cf & ECF_LOOPING_CONST_OR_PURE)))
      return false;
  
    /* We only can sink stmts with a single definition.  */
*************** statement_sink_location (gimple *stmt, b
*** 291,297 ****
    if (stmt_ends_bb_p (stmt)
        || gimple_has_side_effects (stmt)
        || (cfun->has_local_explicit_reg_vars
! 	  && TYPE_MODE (TREE_TYPE (gimple_assign_lhs (stmt))) == BLKmode))
      return false;
  
    /* Return if there are no immediate uses of this stmt.  */
--- 296,302 ----
    if (stmt_ends_bb_p (stmt)
        || gimple_has_side_effects (stmt)
        || (cfun->has_local_explicit_reg_vars
! 	  && TYPE_MODE (TREE_TYPE (gimple_get_lhs (stmt))) == BLKmode))
      return false;
  
    /* Return if there are no immediate uses of this stmt.  */
*************** statement_sink_location (gimple *stmt, b
*** 323,337 ****
  
  	  /* A killing definition is not a use.  */
  	  if ((gimple_has_lhs (use_stmt)
! 	       && operand_equal_p (gimple_assign_lhs (stmt),
  				   gimple_get_lhs (use_stmt), 0))
! 	      || stmt_kills_ref_p (use_stmt, gimple_assign_lhs (stmt)))
  	    {
  	      /* If use_stmt is or might be a nop assignment then USE_STMT
  	         acts as a use as well as definition.  */
  	      if (stmt != use_stmt
  		  && ref_maybe_used_by_stmt_p (use_stmt,
! 					       gimple_assign_lhs (stmt)))
  		return false;
  	      continue;
  	    }
--- 328,342 ----
  
  	  /* A killing definition is not a use.  */
  	  if ((gimple_has_lhs (use_stmt)
! 	       && operand_equal_p (gimple_get_lhs (stmt),
  				   gimple_get_lhs (use_stmt), 0))
! 	      || stmt_kills_ref_p (use_stmt, gimple_get_lhs (stmt)))
  	    {
  	      /* If use_stmt is or might be a nop assignment then USE_STMT
  	         acts as a use as well as definition.  */
  	      if (stmt != use_stmt
  		  && ref_maybe_used_by_stmt_p (use_stmt,
! 					       gimple_get_lhs (stmt)))
  		return false;
  	      continue;
  	    }
Index: gcc/testsuite/gcc.dg/tree-ssa/ssa-sink-16.c
===================================================================
*** gcc/testsuite/gcc.dg/tree-ssa/ssa-sink-16.c	(nonexistent)
--- gcc/testsuite/gcc.dg/tree-ssa/ssa-sink-16.c	(working copy)
***************
*** 0 ****
--- 1,14 ----
+ /* { dg-do compile } */
+ /* Note PRE rotates the loop and blocks the sinking opportunity.  */
+ /* { dg-options "-O2 -fno-tree-pre -fdump-tree-sink -fdump-tree-optimized" } */
+ 
+ int f(int n)
+ {
+   int i,j=0;
+   for (i = 0; i < 31; i++)
+     j = __builtin_ffs(i);
+   return j;
+ }
+ 
+ /* { dg-final { scan-tree-dump "Sinking j_. = __builtin_ffs" "sink" } } */
+ /* { dg-final { scan-tree-dump "return 2;" "optimized" } } */
Index: gcc/testsuite/gcc.target/i386/pr22152.c
===================================================================
*** gcc/testsuite/gcc.target/i386/pr22152.c	(revision 247092)
--- gcc/testsuite/gcc.target/i386/pr22152.c	(working copy)
***************
*** 1,5 ****
  /* { dg-do compile } */
! /* { dg-options "-O2 -msse2 -mtune=core2" } */
  /* { dg-additional-options "-mno-vect8-ret-in-mem" { target *-*-vxworks* } } */
  /* { dg-additional-options "-mabi=sysv" { target x86_64-*-mingw* } } */
  
--- 1,5 ----
  /* { dg-do compile } */
! /* { dg-options "-O2 -fno-tree-sink -msse2 -mtune=core2" } */
  /* { dg-additional-options "-mno-vect8-ret-in-mem" { target *-*-vxworks* } } */
  /* { dg-additional-options "-mabi=sysv" { target x86_64-*-mingw* } } */
  

Re: [PATCH] Fix PR79201 (half-way)

[Rainer Orth]

Hi Richard,

> On Mon, 24 Apr 2017, Richard Biener wrote:
>> 
>> One issue in PR79201 is that we don't sink pure/const calls which is
>> what the following simple patch fixes.
>> 
>> Bootstrap and regtest running on x86_64-unknown-linux-gnu.
>
> Needed some gimple_assign_lhs -> gimple_get_lhs adjustments and
> adjustment of gcc.target/i386/pr22152.c where we now sink the
> assignment out of the pointless loop.  Not sure what the original
> bug was about (well, reg allocation) so I simply disabled sinking
> for it.
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.
>
> Richard.
>
> 2017-04-25  Richard Biener  <rguenther@suse.de>
>
> 	PR tree-optimization/79201
> 	* tree-ssa-sink.c (statement_sink_location): Handle calls.
>
> 	* gcc.dg/tree-ssa/ssa-sink-16.c: New testcase.
> 	* gcc.target/i386/pr22152.c: Disable sinking.

however, gcc.target/i386/pr22152.c FAILs now for 32-bit:

FAIL: gcc.target/i386/pr22152.c scan-assembler-times movq[ \\\\t]+[^\\n]*%mm 1

	Rainer

-- 
-----------------------------------------------------------------------------
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH] Fix PR79201 (half-way)

[Richard Biener]

On Thu, 11 May 2017, Rainer Orth wrote:

> Hi Richard,
> 
> > On Mon, 24 Apr 2017, Richard Biener wrote:
> >> 
> >> One issue in PR79201 is that we don't sink pure/const calls which is
> >> what the following simple patch fixes.
> >> 
> >> Bootstrap and regtest running on x86_64-unknown-linux-gnu.
> >
> > Needed some gimple_assign_lhs -> gimple_get_lhs adjustments and
> > adjustment of gcc.target/i386/pr22152.c where we now sink the
> > assignment out of the pointless loop.  Not sure what the original
> > bug was about (well, reg allocation) so I simply disabled sinking
> > for it.
> >
> > Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.
> >
> > Richard.
> >
> > 2017-04-25  Richard Biener  <rguenther@suse.de>
> >
> > 	PR tree-optimization/79201
> > 	* tree-ssa-sink.c (statement_sink_location): Handle calls.
> >
> > 	* gcc.dg/tree-ssa/ssa-sink-16.c: New testcase.
> > 	* gcc.target/i386/pr22152.c: Disable sinking.
> 
> however, gcc.target/i386/pr22152.c FAILs now for 32-bit:
> 
> FAIL: gcc.target/i386/pr22152.c scan-assembler-times movq[ \\\\t]+[^\\n]*%mm 1

I remember seeing this and was not able to make sense of the testcase
which was added to fix some backend issue.  Disabling sinking doesn't
work (IIRC) as it is required to generate the original code as well.

Uros added the testcase in 2008 -- I think if we want to have a testcase
for the original issue we need a different one.  Or simply remove
the testcase.

Richard.

Re: [PATCH] Fix PR79201 (half-way)

[Uros Bizjak]

On Thu, May 11, 2017 at 2:48 PM, Richard Biener <rguenther@suse.de> wrote:
> On Thu, 11 May 2017, Rainer Orth wrote:
>
>> Hi Richard,
>>
>> > On Mon, 24 Apr 2017, Richard Biener wrote:
>> >>
>> >> One issue in PR79201 is that we don't sink pure/const calls which is
>> >> what the following simple patch fixes.
>> >>
>> >> Bootstrap and regtest running on x86_64-unknown-linux-gnu.
>> >
>> > Needed some gimple_assign_lhs -> gimple_get_lhs adjustments and
>> > adjustment of gcc.target/i386/pr22152.c where we now sink the
>> > assignment out of the pointless loop.  Not sure what the original
>> > bug was about (well, reg allocation) so I simply disabled sinking
>> > for it.
>> >
>> > Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.
>> >
>> > Richard.
>> >
>> > 2017-04-25  Richard Biener  <rguenther@suse.de>
>> >
>> >     PR tree-optimization/79201
>> >     * tree-ssa-sink.c (statement_sink_location): Handle calls.
>> >
>> >     * gcc.dg/tree-ssa/ssa-sink-16.c: New testcase.
>> >     * gcc.target/i386/pr22152.c: Disable sinking.
>>
>> however, gcc.target/i386/pr22152.c FAILs now for 32-bit:
>>
>> FAIL: gcc.target/i386/pr22152.c scan-assembler-times movq[ \\\\t]+[^\\n]*%mm 1
>
> I remember seeing this and was not able to make sense of the testcase
> which was added to fix some backend issue.  Disabling sinking doesn't
> work (IIRC) as it is required to generate the original code as well.
>
> Uros added the testcase in 2008 -- I think if we want to have a testcase
> for the original issue we need a different one.  Or simply remove
> the testcase.

No, there is something going on in the testcase:

.L3:
        movq    (%ecx,%eax,8), %mm1
        paddq   (%ebx,%eax,8), %mm1
        addl    $1, %eax
        movq    %mm1, %mm0
        cmpl    %eax, %edx
        jne     .L3


The compiler should allocate %mm0 to movq and paddq to avoid %mm1 ->
%mm0 move. These are all movv1di patterns (they shouldn't interfere
with movdi), and it is not clear to me why RA allocates %mm1 instead
of %mm0.

Uros.

Re: [PATCH] Fix PR79201 (half-way)

[Richard Biener]

On Thu, 11 May 2017, Uros Bizjak wrote:

> On Thu, May 11, 2017 at 2:48 PM, Richard Biener <rguenther@suse.de> wrote:
> > On Thu, 11 May 2017, Rainer Orth wrote:
> >
> >> Hi Richard,
> >>
> >> > On Mon, 24 Apr 2017, Richard Biener wrote:
> >> >>
> >> >> One issue in PR79201 is that we don't sink pure/const calls which is
> >> >> what the following simple patch fixes.
> >> >>
> >> >> Bootstrap and regtest running on x86_64-unknown-linux-gnu.
> >> >
> >> > Needed some gimple_assign_lhs -> gimple_get_lhs adjustments and
> >> > adjustment of gcc.target/i386/pr22152.c where we now sink the
> >> > assignment out of the pointless loop.  Not sure what the original
> >> > bug was about (well, reg allocation) so I simply disabled sinking
> >> > for it.
> >> >
> >> > Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.
> >> >
> >> > Richard.
> >> >
> >> > 2017-04-25  Richard Biener  <rguenther@suse.de>
> >> >
> >> >     PR tree-optimization/79201
> >> >     * tree-ssa-sink.c (statement_sink_location): Handle calls.
> >> >
> >> >     * gcc.dg/tree-ssa/ssa-sink-16.c: New testcase.
> >> >     * gcc.target/i386/pr22152.c: Disable sinking.
> >>
> >> however, gcc.target/i386/pr22152.c FAILs now for 32-bit:
> >>
> >> FAIL: gcc.target/i386/pr22152.c scan-assembler-times movq[ \\\\t]+[^\\n]*%mm 1
> >
> > I remember seeing this and was not able to make sense of the testcase
> > which was added to fix some backend issue.  Disabling sinking doesn't
> > work (IIRC) as it is required to generate the original code as well.
> >
> > Uros added the testcase in 2008 -- I think if we want to have a testcase
> > for the original issue we need a different one.  Or simply remove
> > the testcase.
> 
> No, there is something going on in the testcase:
> 
> .L3:
>         movq    (%ecx,%eax,8), %mm1
>         paddq   (%ebx,%eax,8), %mm1
>         addl    $1, %eax
>         movq    %mm1, %mm0
>         cmpl    %eax, %edx
>         jne     .L3
> 
> 
> The compiler should allocate %mm0 to movq and paddq to avoid %mm1 ->
> %mm0 move. These are all movv1di patterns (they shouldn't interfere
> with movdi), and it is not clear to me why RA allocates %mm1 instead
> of %mm0.

In any case the testcase is no longer testing what it tested as the
input to RA is now different.  The testcase doesn't make much sense:

__m64
unsigned_add3 (const __m64 * a, const __m64 * b, unsigned int count)
{
  __m64 sum;
  unsigned int i;

  for (i = 1; i < count; i++)
    sum = _mm_add_si64 (a[i], b[i]);

  return sum;
}

that's equivalent to

__m64
unsigned_add3 (const __m64 * a, const __m64 * b, unsigned int count)
{
  __m64 sum;
  unsigned int i;
   
  if (1 < count)
    sum = _mm_add_si64 (a[count-1], b[count-1]);

  return sum;
}

which means possibly using uninitialized sum plus a pointless loop.

Richard.

Re: [PATCH] Fix PR79201 (half-way)

[Uros Bizjak]

On Thu, May 11, 2017 at 4:02 PM, Richard Biener <rguenther@suse.de> wrote:

>> > Uros added the testcase in 2008 -- I think if we want to have a testcase
>> > for the original issue we need a different one.  Or simply remove
>> > the testcase.
>>
>> No, there is something going on in the testcase:
>>
>> .L3:
>>         movq    (%ecx,%eax,8), %mm1
>>         paddq   (%ebx,%eax,8), %mm1
>>         addl    $1, %eax
>>         movq    %mm1, %mm0
>>         cmpl    %eax, %edx
>>         jne     .L3
>>
>>
>> The compiler should allocate %mm0 to movq and paddq to avoid %mm1 ->
>> %mm0 move. These are all movv1di patterns (they shouldn't interfere
>> with movdi), and it is not clear to me why RA allocates %mm1 instead
>> of %mm0.
>
> In any case the testcase is no longer testing what it tested as the
> input to RA is now different.  The testcase doesn't make much sense:

Following is the cleaned testcase:

--cut here--
/* { dg-do compile { target ia32 } } */
/* { dg-options "-O2 -msse2 -mtune=core2" } */
/* { dg-additional-options "-mno-vect8-ret-in-mem" { target *-*-vxworks* } } */
/* { dg-additional-options "-mabi=sysv" { target x86_64-*-mingw* } } */

#include <mmintrin.h>

typedef __SIZE_TYPE__ size_t;

__m64
unsigned_add3 (const __m64 * a, const __m64 * b, size_t count)
{
  __m64 sum = { 0, 0 };

  if (count > 0)
    sum = _mm_add_si64 (a[count-1], b[count-1]);

  return sum;
}

/* { dg-final { scan-assembler-times "movq\[ \\t\]+\[^\n\]*%mm" 1 } } */
-- cut here--

The testcase still tests that only one movq is generated (gcc-4.1
generated three). However, I have disabled the test on x86_64, since
x86_64 returns mmx values in XMM registers, and MMX -> XMM moves
always go through memory.

Uros.

Re: [PATCH] Fix PR79201 (half-way)

[Richard Biener]

On Thu, 11 May 2017, Uros Bizjak wrote:

> On Thu, May 11, 2017 at 4:02 PM, Richard Biener <rguenther@suse.de> wrote:
> 
> >> > Uros added the testcase in 2008 -- I think if we want to have a testcase
> >> > for the original issue we need a different one.  Or simply remove
> >> > the testcase.
> >>
> >> No, there is something going on in the testcase:
> >>
> >> .L3:
> >>         movq    (%ecx,%eax,8), %mm1
> >>         paddq   (%ebx,%eax,8), %mm1
> >>         addl    $1, %eax
> >>         movq    %mm1, %mm0
> >>         cmpl    %eax, %edx
> >>         jne     .L3
> >>
> >>
> >> The compiler should allocate %mm0 to movq and paddq to avoid %mm1 ->
> >> %mm0 move. These are all movv1di patterns (they shouldn't interfere
> >> with movdi), and it is not clear to me why RA allocates %mm1 instead
> >> of %mm0.
> >
> > In any case the testcase is no longer testing what it tested as the
> > input to RA is now different.  The testcase doesn't make much sense:
> 
> Following is the cleaned testcase:
> 
> --cut here--
> /* { dg-do compile { target ia32 } } */
> /* { dg-options "-O2 -msse2 -mtune=core2" } */
> /* { dg-additional-options "-mno-vect8-ret-in-mem" { target *-*-vxworks* } } */
> /* { dg-additional-options "-mabi=sysv" { target x86_64-*-mingw* } } */
> 
> #include <mmintrin.h>
> 
> typedef __SIZE_TYPE__ size_t;
> 
> __m64
> unsigned_add3 (const __m64 * a, const __m64 * b, size_t count)
> {
>   __m64 sum = { 0, 0 };
> 
>   if (count > 0)
>     sum = _mm_add_si64 (a[count-1], b[count-1]);
> 
>   return sum;
> }
> 
> /* { dg-final { scan-assembler-times "movq\[ \\t\]+\[^\n\]*%mm" 1 } } */
> -- cut here--
> 
> The testcase still tests that only one movq is generated (gcc-4.1
> generated three). However, I have disabled the test on x86_64, since
> x86_64 returns mmx values in XMM registers, and MMX -> XMM moves
> always go through memory.

Thank you very much.

Richard.