Fix PR83491

Message ID DB6PR0801MB20534A452D705CDB3BA7FF05830C0@DB6PR0801MB2053.eurprd08.prod.outlook.com
State New
Headers show
Series
  • Fix PR83491
Related show

Commit Message

Wilco Dijkstra Dec. 20, 2017, 1:56 p.m.
This patch fixes PR83491 - if an SSA expression contains 2 identical float
constants, the division reciprocal optimization will ICE.  Fix this by explicitly
checking for SSA_NAME in the tree code before walking the uses.  Also fix several
coding style issues pointed out by Jakub and make comments more readable.

Bootstrap OK, OK for trunk?

ChangeLog:
2017-12-20  Wilco Dijkstra  <wdijkstr@arm.com>

    gcc/
	PR tree-optimization/83491
	* tree-ssa-math-opts.c (execute_cse_reciprocals_1): Check for SSA_NAME
	before walking uses.  Improve coding style and comments.

    gcc/testsuite/
	PR tree-optimization/83491
	* gcc.dg/pr83491.c: Add new test.
--

Comments

Jakub Jelinek Dec. 20, 2017, 1:59 p.m. | #1
On Wed, Dec 20, 2017 at 01:56:27PM +0000, Wilco Dijkstra wrote:
> This patch fixes PR83491 - if an SSA expression contains 2 identical float

> constants, the division reciprocal optimization will ICE.  Fix this by explicitly

> checking for SSA_NAME in the tree code before walking the uses.  Also fix several

> coding style issues pointed out by Jakub and make comments more readable.

> 

> Bootstrap OK, OK for trunk?

> 

> ChangeLog:

> 2017-12-20  Wilco Dijkstra  <wdijkstr@arm.com>

> 

>     gcc/

> 	PR tree-optimization/83491

> 	* tree-ssa-math-opts.c (execute_cse_reciprocals_1): Check for SSA_NAME

> 	before walking uses.  Improve coding style and comments.

> 

>     gcc/testsuite/

> 	PR tree-optimization/83491

> 	* gcc.dg/pr83491.c: Add new test.


Ok, thanks.

	Jakub
Jeff Law Dec. 20, 2017, 11:56 p.m. | #2
On 12/20/2017 06:59 AM, Jakub Jelinek wrote:
> On Wed, Dec 20, 2017 at 01:56:27PM +0000, Wilco Dijkstra wrote:

>> This patch fixes PR83491 - if an SSA expression contains 2 identical float

>> constants, the division reciprocal optimization will ICE.  Fix this by explicitly

>> checking for SSA_NAME in the tree code before walking the uses.  Also fix several

>> coding style issues pointed out by Jakub and make comments more readable.

>>

>> Bootstrap OK, OK for trunk?

>>

>> ChangeLog:

>> 2017-12-20  Wilco Dijkstra  <wdijkstr@arm.com>

>>

>>     gcc/

>> 	PR tree-optimization/83491

>> 	* tree-ssa-math-opts.c (execute_cse_reciprocals_1): Check for SSA_NAME

>> 	before walking uses.  Improve coding style and comments.

>>

>>     gcc/testsuite/

>> 	PR tree-optimization/83491

>> 	* gcc.dg/pr83491.c: Add new test.

> 

> Ok, thanks.

And I went ahead and committted.

jeff
Richard Biener Jan. 2, 2018, 1:19 p.m. | #3
On Wed, Dec 20, 2017 at 2:56 PM, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:
> This patch fixes PR83491 - if an SSA expression contains 2 identical float

> constants, the division reciprocal optimization will ICE.  Fix this by explicitly

> checking for SSA_NAME in the tree code before walking the uses.  Also fix several

> coding style issues pointed out by Jakub and make comments more readable.

>

> Bootstrap OK, OK for trunk?

>

> ChangeLog:

> 2017-12-20  Wilco Dijkstra  <wdijkstr@arm.com>

>

>     gcc/

>         PR tree-optimization/83491

>         * tree-ssa-math-opts.c (execute_cse_reciprocals_1): Check for SSA_NAME

>         before walking uses.  Improve coding style and comments.

>

>     gcc/testsuite/

>         PR tree-optimization/83491

>         * gcc.dg/pr83491.c: Add new test.

> --

>

> diff --git a/gcc/testsuite/gcc.dg/pr83491.c b/gcc/testsuite/gcc.dg/pr83491.c

> new file mode 100644

> index 0000000000000000000000000000000000000000..f23cc19c72f57ca8d34f05f28fee75fc2c13f33a

> --- /dev/null

> +++ b/gcc/testsuite/gcc.dg/pr83491.c

> @@ -0,0 +1,10 @@

> +/* { dg-do compile } */

> +/* { dg-options "-O1 -funsafe-math-optimizations" } */

> +

> +float a;

> +float b;

> +void bar ()

> +{

> +  a = __builtin_nanf ("");

> +  b = __builtin_powf (a, 2.5F);

> +}

> diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c

> index 8db12f5e1cd5d227bd6c3780420e93cd3fe7b435..4e8f5e728d0c8ef5ac564d8c3c999d8a6ff15e7e 100644

> --- a/gcc/tree-ssa-math-opts.c

> +++ b/gcc/tree-ssa-math-opts.c

> @@ -544,29 +544,32 @@ execute_cse_reciprocals_1 (gimple_stmt_iterator *def_gsi, tree def)

>    int square_recip_count = 0;

>    int sqrt_recip_count = 0;

>

> -  gcc_assert (FLOAT_TYPE_P (TREE_TYPE (def)) && is_gimple_reg (def));

> +  gcc_assert (FLOAT_TYPE_P (TREE_TYPE (def)) && is_gimple_reg (def)

> +             && TREE_CODE (def) == SSA_NAME);


The is_gimple_reg () predicate test is now redundant.

>    threshold = targetm.min_divisions_for_recip_mul (TYPE_MODE (TREE_TYPE (def)));

>

> -  /* If this is a square (x * x), we should check whether there are any

> -     enough divisions by x on it's own to warrant waiting for that pass.  */

> -  if (TREE_CODE (def) == SSA_NAME)

> +  /* If DEF is a square (x * x), count the number of divisions by x.

> +     If there are more divisions by x than by (DEF * DEF), prefer to optimize

> +     the reciprocal of x instead of DEF.  This improves cases like:

> +       def = x * x

> +       t0 = a / def

> +       t1 = b / def

> +       t2 = c / x

> +     Reciprocal optimization of x results in 1 division rather than 2 or 3.  */

> +  gimple *def_stmt = SSA_NAME_DEF_STMT (def);

> +

> +  if (is_gimple_assign (def_stmt)

> +      && gimple_assign_rhs_code (def_stmt) == MULT_EXPR

> +      && TREE_CODE (gimple_assign_rhs1 (def_stmt)) == SSA_NAME

> +      && gimple_assign_rhs1 (def_stmt) == gimple_assign_rhs2 (def_stmt))

>      {

> -      gimple *def_stmt = SSA_NAME_DEF_STMT (def);

> +      tree op0 = gimple_assign_rhs1 (def_stmt);

>

> -      if (is_gimple_assign (def_stmt)

> -         && gimple_assign_rhs_code (def_stmt) == MULT_EXPR

> -         && gimple_assign_rhs1 (def_stmt) == gimple_assign_rhs2 (def_stmt))

> +      FOR_EACH_IMM_USE_FAST (use_p, use_iter, op0)

>         {

> -         /* This statement is a square of something.  We should take this

> -            in to account, as it may be more profitable to not extract

> -            the reciprocal here.  */

> -         tree op0 = gimple_assign_rhs1 (def_stmt);

> -         FOR_EACH_IMM_USE_FAST (use_p, use_iter, op0)

> -           {

> -             gimple *use_stmt = USE_STMT (use_p);

> -             if (is_division_by (use_stmt, op0))

> -               sqrt_recip_count ++;

> -           }

> +         gimple *use_stmt = USE_STMT (use_p);

> +         if (is_division_by (use_stmt, op0))

> +           sqrt_recip_count++;

>         }

>      }

>

> @@ -587,26 +590,23 @@ execute_cse_reciprocals_1 (gimple_stmt_iterator *def_gsi, tree def)

>               gimple *square_use_stmt = USE_STMT (square_use_p);

>               if (is_division_by (square_use_stmt, square_def))

>                 {

> -                 /* Halve the relative importance as this is called twice

> -                    for each division by a square.  */

> +                 /* This is executed twice for each division by a square.  */

>                   register_division_in (gimple_bb (square_use_stmt), 1);

> -                 square_recip_count ++;

> +                 square_recip_count++;

>                 }

>             }

>         }

>      }

>

> -  /* Square reciprocals will have been counted twice.  */

> +  /* Square reciprocals were counted twice above.  */

>    square_recip_count /= 2;

>

> +  /* If it is more profitable to optimize 1 / x, don't optimize 1 / (x * x).  */

>    if (sqrt_recip_count > square_recip_count)

> -    /* It will be more profitable to extract a 1 / x expression later,

> -       so it is not worth attempting to extract 1 / (x * x) now.  */

>      return;

>

>    /* Do the expensive part only if we can hope to optimize something.  */

> -  if (count + square_recip_count >= threshold

> -      && count >= 1)

> +  if (count + square_recip_count >= threshold && count >= 1)

>      {

>        gimple *use_stmt;

>        for (occ = occ_head; occ; occ = occ->next)

> @@ -623,8 +623,7 @@ execute_cse_reciprocals_1 (gimple_stmt_iterator *def_gsi, tree def)

>               FOR_EACH_IMM_USE_ON_STMT (use_p, use_iter)

>                 replace_reciprocal (use_p);

>             }

> -         else if (square_recip_count > 0

> -                  && is_square_of (use_stmt, def))

> +         else if (square_recip_count > 0 && is_square_of (use_stmt, def))

>             {

>               FOR_EACH_IMM_USE_ON_STMT (use_p, use_iter)

>                 {
Jeff Law Jan. 4, 2018, 6:52 p.m. | #4
On 01/02/2018 06:19 AM, Richard Biener wrote:
> On Wed, Dec 20, 2017 at 2:56 PM, Wilco Dijkstra <Wilco.Dijkstra@arm.com> wrote:

>> This patch fixes PR83491 - if an SSA expression contains 2 identical float

>> constants, the division reciprocal optimization will ICE.  Fix this by explicitly

>> checking for SSA_NAME in the tree code before walking the uses.  Also fix several

>> coding style issues pointed out by Jakub and make comments more readable.

>>

>> Bootstrap OK, OK for trunk?

>>

>> ChangeLog:

>> 2017-12-20  Wilco Dijkstra  <wdijkstr@arm.com>

>>

>>     gcc/

>>         PR tree-optimization/83491

>>         * tree-ssa-math-opts.c (execute_cse_reciprocals_1): Check for SSA_NAME

>>         before walking uses.  Improve coding style and comments.

>>

>>     gcc/testsuite/

>>         PR tree-optimization/83491

>>         * gcc.dg/pr83491.c: Add new test.

>> --

>>

>> diff --git a/gcc/testsuite/gcc.dg/pr83491.c b/gcc/testsuite/gcc.dg/pr83491.c

>> new file mode 100644

>> index 0000000000000000000000000000000000000000..f23cc19c72f57ca8d34f05f28fee75fc2c13f33a

>> --- /dev/null

>> +++ b/gcc/testsuite/gcc.dg/pr83491.c

>> @@ -0,0 +1,10 @@

>> +/* { dg-do compile } */

>> +/* { dg-options "-O1 -funsafe-math-optimizations" } */

>> +

>> +float a;

>> +float b;

>> +void bar ()

>> +{

>> +  a = __builtin_nanf ("");

>> +  b = __builtin_powf (a, 2.5F);

>> +}

>> diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c

>> index 8db12f5e1cd5d227bd6c3780420e93cd3fe7b435..4e8f5e728d0c8ef5ac564d8c3c999d8a6ff15e7e 100644

>> --- a/gcc/tree-ssa-math-opts.c

>> +++ b/gcc/tree-ssa-math-opts.c

>> @@ -544,29 +544,32 @@ execute_cse_reciprocals_1 (gimple_stmt_iterator *def_gsi, tree def)

>>    int square_recip_count = 0;

>>    int sqrt_recip_count = 0;

>>

>> -  gcc_assert (FLOAT_TYPE_P (TREE_TYPE (def)) && is_gimple_reg (def));

>> +  gcc_assert (FLOAT_TYPE_P (TREE_TYPE (def)) && is_gimple_reg (def)

>> +             && TREE_CODE (def) == SSA_NAME);

> 

> The is_gimple_reg () predicate test is now redundant.

Agreed.  I actually removed it in one of my recent bootstrap/regression
test cycles.  I'll commit the removal momentarily.

jeff

Patch

diff --git a/gcc/testsuite/gcc.dg/pr83491.c b/gcc/testsuite/gcc.dg/pr83491.c
new file mode 100644
index 0000000000000000000000000000000000000000..f23cc19c72f57ca8d34f05f28fee75fc2c13f33a
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/pr83491.c
@@ -0,0 +1,10 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O1 -funsafe-math-optimizations" } */
+
+float a;
+float b;
+void bar ()
+{
+  a = __builtin_nanf ("");
+  b = __builtin_powf (a, 2.5F);
+}
diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c
index 8db12f5e1cd5d227bd6c3780420e93cd3fe7b435..4e8f5e728d0c8ef5ac564d8c3c999d8a6ff15e7e 100644
--- a/gcc/tree-ssa-math-opts.c
+++ b/gcc/tree-ssa-math-opts.c
@@ -544,29 +544,32 @@  execute_cse_reciprocals_1 (gimple_stmt_iterator *def_gsi, tree def)
   int square_recip_count = 0;
   int sqrt_recip_count = 0;
 
-  gcc_assert (FLOAT_TYPE_P (TREE_TYPE (def)) && is_gimple_reg (def));
+  gcc_assert (FLOAT_TYPE_P (TREE_TYPE (def)) && is_gimple_reg (def)
+	      && TREE_CODE (def) == SSA_NAME);
   threshold = targetm.min_divisions_for_recip_mul (TYPE_MODE (TREE_TYPE (def)));
 
-  /* If this is a square (x * x), we should check whether there are any
-     enough divisions by x on it's own to warrant waiting for that pass.  */
-  if (TREE_CODE (def) == SSA_NAME)
+  /* If DEF is a square (x * x), count the number of divisions by x.
+     If there are more divisions by x than by (DEF * DEF), prefer to optimize
+     the reciprocal of x instead of DEF.  This improves cases like:
+       def = x * x
+       t0 = a / def
+       t1 = b / def
+       t2 = c / x
+     Reciprocal optimization of x results in 1 division rather than 2 or 3.  */
+  gimple *def_stmt = SSA_NAME_DEF_STMT (def);
+
+  if (is_gimple_assign (def_stmt)
+      && gimple_assign_rhs_code (def_stmt) == MULT_EXPR
+      && TREE_CODE (gimple_assign_rhs1 (def_stmt)) == SSA_NAME
+      && gimple_assign_rhs1 (def_stmt) == gimple_assign_rhs2 (def_stmt))
     {
-      gimple *def_stmt = SSA_NAME_DEF_STMT (def);
+      tree op0 = gimple_assign_rhs1 (def_stmt);
 
-      if (is_gimple_assign (def_stmt)
-	  && gimple_assign_rhs_code (def_stmt) == MULT_EXPR
-	  && gimple_assign_rhs1 (def_stmt) == gimple_assign_rhs2 (def_stmt))
+      FOR_EACH_IMM_USE_FAST (use_p, use_iter, op0)
 	{
-	  /* This statement is a square of something.  We should take this
-	     in to account, as it may be more profitable to not extract
-	     the reciprocal here.  */
-	  tree op0 = gimple_assign_rhs1 (def_stmt);
-	  FOR_EACH_IMM_USE_FAST (use_p, use_iter, op0)
-	    {
-	      gimple *use_stmt = USE_STMT (use_p);
-	      if (is_division_by (use_stmt, op0))
-		sqrt_recip_count ++;
-	    }
+	  gimple *use_stmt = USE_STMT (use_p);
+	  if (is_division_by (use_stmt, op0))
+	    sqrt_recip_count++;
 	}
     }
 
@@ -587,26 +590,23 @@  execute_cse_reciprocals_1 (gimple_stmt_iterator *def_gsi, tree def)
 	      gimple *square_use_stmt = USE_STMT (square_use_p);
 	      if (is_division_by (square_use_stmt, square_def))
 		{
-		  /* Halve the relative importance as this is called twice
-		     for each division by a square.  */
+		  /* This is executed twice for each division by a square.  */
 		  register_division_in (gimple_bb (square_use_stmt), 1);
-		  square_recip_count ++;
+		  square_recip_count++;
 		}
 	    }
 	}
     }
 
-  /* Square reciprocals will have been counted twice.  */
+  /* Square reciprocals were counted twice above.  */
   square_recip_count /= 2;
 
+  /* If it is more profitable to optimize 1 / x, don't optimize 1 / (x * x).  */
   if (sqrt_recip_count > square_recip_count)
-    /* It will be more profitable to extract a 1 / x expression later,
-       so it is not worth attempting to extract 1 / (x * x) now.  */
     return;
 
   /* Do the expensive part only if we can hope to optimize something.  */
-  if (count + square_recip_count >= threshold
-      && count >= 1)
+  if (count + square_recip_count >= threshold && count >= 1)
     {
       gimple *use_stmt;
       for (occ = occ_head; occ; occ = occ->next)
@@ -623,8 +623,7 @@  execute_cse_reciprocals_1 (gimple_stmt_iterator *def_gsi, tree def)
 	      FOR_EACH_IMM_USE_ON_STMT (use_p, use_iter)
 		replace_reciprocal (use_p);
 	    }
-	  else if (square_recip_count > 0
-		   && is_square_of (use_stmt, def))
+	  else if (square_recip_count > 0 && is_square_of (use_stmt, def))
 	    {
 	      FOR_EACH_IMM_USE_ON_STMT (use_p, use_iter)
 		{