[PATCH] Fix PR54240

[PATCH] Fix PR54240

[William J. Schmidt]

Replace the once vacuously true, and now vacuously false, test for
existence of a conditional move instruction for a given mode, with one
that actually checks what it's supposed to.  Add a test case so we don't
miss such things in future.

The test is powerpc-specific.  It would be good to have an i386 version
of the test as well, if someone can help with that.

Bootstrapped and tested on powerpc64-unknown-linux-gnu with no new
regressions.  Ok for trunk?

Thanks,
Bill


gcc:

2012-08-13  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	PR tree-optimization/54240
	* tree-ssa-phiopt.c (hoist_adjacent_loads): Correct test for
	existence of conditional move with given mode.


gcc/testsuite:

2012-08-13  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>

	PR tree-optimization/54240
	* gcc.target/powerpc/pr54240.c: New test.


Index: gcc/testsuite/gcc.target/powerpc/pr54240.c
===================================================================
--- gcc/testsuite/gcc.target/powerpc/pr54240.c	(revision 0)
+++ gcc/testsuite/gcc.target/powerpc/pr54240.c	(revision 0)
@@ -0,0 +1,27 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -misel -fdump-tree-phiopt-details" } */
+
+typedef struct s {
+  int v;
+  int b;
+  struct s *l;
+  struct s *r;
+} S;
+
+
+int foo(S *s)
+{
+  S *this;
+  S *next;
+
+  this = s;
+  if (this->b)
+    next = this->l;
+  else
+    next = this->r;
+
+  return next->v;
+}
+
+/* { dg-final { scan-tree-dump "Hoisting adjacent loads" "phiopt1" } } */
+/* { dg-final { cleanup-tree-dump "phiopt1" } } */
Index: gcc/tree-ssa-phiopt.c
===================================================================
--- gcc/tree-ssa-phiopt.c	(revision 190305)
+++ gcc/tree-ssa-phiopt.c	(working copy)
@@ -1843,7 +1843,8 @@ hoist_adjacent_loads (basic_block bb0, basic_block
 
       /* Check the mode of the arguments to be sure a conditional move
 	 can be generated for it.  */
-      if (!optab_handler (cmov_optab, TYPE_MODE (TREE_TYPE (arg1))))
+      if (optab_handler (movcc_optab, TYPE_MODE (TREE_TYPE (arg1)))
+	  == CODE_FOR_nothing)
 	continue;
 
       /* Both statements must be assignments whose RHS is a COMPONENT_REF.  */

Re: [PATCH] Fix PR54240

[Andrew Pinski]

On Tue, Aug 14, 2012 at 2:11 PM, William J. Schmidt
<wschmidt@linux.vnet.ibm.com> wrote:
> Replace the once vacuously true, and now vacuously false, test for
> existence of a conditional move instruction for a given mode, with one
> that actually checks what it's supposed to.  Add a test case so we don't
> miss such things in future.
>
> The test is powerpc-specific.  It would be good to have an i386 version
> of the test as well, if someone can help with that.
>
> Bootstrapped and tested on powerpc64-unknown-linux-gnu with no new
> regressions.  Ok for trunk?

Here is one which can go into gcc.target/mips :
/* { dg-do compile } */
/* { dg-options "-O2 -fdump-tree-phiopt-details" } */

typedef struct s {
  int v;
  int b;
  struct s *l;
  struct s *r;
} S;


int foo(S *s)
{
  S *this;
  S *next;

  this = s;
  if (this->b)
    next = this->l;
  else
    next = this->r;

  return next->v;
}

/* { dg-final { scan-tree-dump "Hoisting adjacent loads" "phiopt1" } } */
/* { dg-final { cleanup-tree-dump "phiopt1" } } */

Re: [PATCH] Fix PR54240

[Andrew Pinski]

On Tue, Aug 14, 2012 at 2:15 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Tue, Aug 14, 2012 at 2:11 PM, William J. Schmidt
> <wschmidt@linux.vnet.ibm.com> wrote:
>> Replace the once vacuously true, and now vacuously false, test for
>> existence of a conditional move instruction for a given mode, with one
>> that actually checks what it's supposed to.  Add a test case so we don't
>> miss such things in future.
>>
>> The test is powerpc-specific.  It would be good to have an i386 version
>> of the test as well, if someone can help with that.
>>
>> Bootstrapped and tested on powerpc64-unknown-linux-gnu with no new
>> regressions.  Ok for trunk?
>
> Here is one which can go into gcc.target/mips :
> /* { dg-do compile } */
> /* { dg-options "-O2 -fdump-tree-phiopt-details" } */

Sorry the dg-options should be:
 /* { dg-options "-O2 -fdump-tree-phiopt-details isa>=4" } */

Thanks,
Andrew

>
> typedef struct s {
>   int v;
>   int b;
>   struct s *l;
>   struct s *r;
> } S;
>
>
> int foo(S *s)
> {
>   S *this;
>   S *next;
>
>   this = s;
>   if (this->b)
>     next = this->l;
>   else
>     next = this->r;
>
>   return next->v;
> }
>
> /* { dg-final { scan-tree-dump "Hoisting adjacent loads" "phiopt1" } } */
> /* { dg-final { cleanup-tree-dump "phiopt1" } } */

Re: [PATCH] Fix PR54240

[William J. Schmidt]

Thanks, Andrew!

Bill

On Tue, 2012-08-14 at 14:17 -0700, Andrew Pinski wrote:
> On Tue, Aug 14, 2012 at 2:15 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> > On Tue, Aug 14, 2012 at 2:11 PM, William J. Schmidt
> > <wschmidt@linux.vnet.ibm.com> wrote:
> >> Replace the once vacuously true, and now vacuously false, test for
> >> existence of a conditional move instruction for a given mode, with one
> >> that actually checks what it's supposed to.  Add a test case so we don't
> >> miss such things in future.
> >>
> >> The test is powerpc-specific.  It would be good to have an i386 version
> >> of the test as well, if someone can help with that.
> >>
> >> Bootstrapped and tested on powerpc64-unknown-linux-gnu with no new
> >> regressions.  Ok for trunk?
> >
> > Here is one which can go into gcc.target/mips :
> > /* { dg-do compile } */
> > /* { dg-options "-O2 -fdump-tree-phiopt-details" } */
> 
> Sorry the dg-options should be:
>  /* { dg-options "-O2 -fdump-tree-phiopt-details isa>=4" } */
> 
> Thanks,
> Andrew
> 
> >
> > typedef struct s {
> >   int v;
> >   int b;
> >   struct s *l;
> >   struct s *r;
> > } S;
> >
> >
> > int foo(S *s)
> > {
> >   S *this;
> >   S *next;
> >
> >   this = s;
> >   if (this->b)
> >     next = this->l;
> >   else
> >     next = this->r;
> >
> >   return next->v;
> > }
> >
> > /* { dg-final { scan-tree-dump "Hoisting adjacent loads" "phiopt1" } } */
> > /* { dg-final { cleanup-tree-dump "phiopt1" } } */
> 

Re: [PATCH] Fix PR54240

[Richard Guenther]

On Tue, 14 Aug 2012, William J. Schmidt wrote:

> Replace the once vacuously true, and now vacuously false, test for
> existence of a conditional move instruction for a given mode, with one
> that actually checks what it's supposed to.  Add a test case so we don't
> miss such things in future.
> 
> The test is powerpc-specific.  It would be good to have an i386 version
> of the test as well, if someone can help with that.
> 
> Bootstrapped and tested on powerpc64-unknown-linux-gnu with no new
> regressions.  Ok for trunk?

Ok if you also drop in the testcase from Andrew.

Thanks,
Richard.

> Thanks,
> Bill
> 
> 
> gcc:
> 
> 2012-08-13  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
> 
> 	PR tree-optimization/54240
> 	* tree-ssa-phiopt.c (hoist_adjacent_loads): Correct test for
> 	existence of conditional move with given mode.
> 
> 
> gcc/testsuite:
> 
> 2012-08-13  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
> 
> 	PR tree-optimization/54240
> 	* gcc.target/powerpc/pr54240.c: New test.
> 
> 
> Index: gcc/testsuite/gcc.target/powerpc/pr54240.c
> ===================================================================
> --- gcc/testsuite/gcc.target/powerpc/pr54240.c	(revision 0)
> +++ gcc/testsuite/gcc.target/powerpc/pr54240.c	(revision 0)
> @@ -0,0 +1,27 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -misel -fdump-tree-phiopt-details" } */
> +
> +typedef struct s {
> +  int v;
> +  int b;
> +  struct s *l;
> +  struct s *r;
> +} S;
> +
> +
> +int foo(S *s)
> +{
> +  S *this;
> +  S *next;
> +
> +  this = s;
> +  if (this->b)
> +    next = this->l;
> +  else
> +    next = this->r;
> +
> +  return next->v;
> +}
> +
> +/* { dg-final { scan-tree-dump "Hoisting adjacent loads" "phiopt1" } } */
> +/* { dg-final { cleanup-tree-dump "phiopt1" } } */
> Index: gcc/tree-ssa-phiopt.c
> ===================================================================
> --- gcc/tree-ssa-phiopt.c	(revision 190305)
> +++ gcc/tree-ssa-phiopt.c	(working copy)
> @@ -1843,7 +1843,8 @@ hoist_adjacent_loads (basic_block bb0, basic_block
>  
>        /* Check the mode of the arguments to be sure a conditional move
>  	 can be generated for it.  */
> -      if (!optab_handler (cmov_optab, TYPE_MODE (TREE_TYPE (arg1))))
> +      if (optab_handler (movcc_optab, TYPE_MODE (TREE_TYPE (arg1)))
> +	  == CODE_FOR_nothing)
>  	continue;
>  
>        /* Both statements must be assignments whose RHS is a COMPONENT_REF.  */
> 
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imend