* [PATCH] Fix PR83491
@ 2017-12-20 13:56 Wilco Dijkstra
2017-12-20 13:59 ` Jakub Jelinek
2018-01-02 13:19 ` Richard Biener
0 siblings, 2 replies; 5+ messages in thread
From: Wilco Dijkstra @ 2017-12-20 13:56 UTC (permalink / raw)
To: GCC Patches; +Cc: Jakub Jelinek, nd
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);
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)
{
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix PR83491
2017-12-20 13:56 [PATCH] Fix PR83491 Wilco Dijkstra
@ 2017-12-20 13:59 ` Jakub Jelinek
2017-12-20 23:56 ` Jeff Law
2018-01-02 13:19 ` Richard Biener
1 sibling, 1 reply; 5+ messages in thread
From: Jakub Jelinek @ 2017-12-20 13:59 UTC (permalink / raw)
To: Wilco Dijkstra; +Cc: GCC Patches, nd
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix PR83491
2017-12-20 13:59 ` Jakub Jelinek
@ 2017-12-20 23:56 ` Jeff Law
0 siblings, 0 replies; 5+ messages in thread
From: Jeff Law @ 2017-12-20 23:56 UTC (permalink / raw)
To: Jakub Jelinek, Wilco Dijkstra; +Cc: GCC Patches, nd
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix PR83491
2017-12-20 13:56 [PATCH] Fix PR83491 Wilco Dijkstra
2017-12-20 13:59 ` Jakub Jelinek
@ 2018-01-02 13:19 ` Richard Biener
2018-01-04 18:52 ` Jeff Law
1 sibling, 1 reply; 5+ messages in thread
From: Richard Biener @ 2018-01-02 13:19 UTC (permalink / raw)
To: Wilco Dijkstra; +Cc: GCC Patches, Jakub Jelinek, nd
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)
> {
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Fix PR83491
2018-01-02 13:19 ` Richard Biener
@ 2018-01-04 18:52 ` Jeff Law
0 siblings, 0 replies; 5+ messages in thread
From: Jeff Law @ 2018-01-04 18:52 UTC (permalink / raw)
To: Richard Biener, Wilco Dijkstra; +Cc: GCC Patches, Jakub Jelinek, nd
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-01-04 18:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-20 13:56 [PATCH] Fix PR83491 Wilco Dijkstra
2017-12-20 13:59 ` Jakub Jelinek
2017-12-20 23:56 ` Jeff Law
2018-01-02 13:19 ` Richard Biener
2018-01-04 18:52 ` Jeff Law
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).