Patchwork PR57073 - Optimize __builtin_powif (-1.0, k) to k & 1 ? -1.0 : 1.0

login
register
mail settings
Submitter Tobias Burnus
Date May 30, 2013, 6:08 p.m.
Message ID <51A79580.2010001@net-b.de>
Download mbox | patch
Permalink /patch/247663/
State New
Headers show

Comments

Tobias Burnus - May 30, 2013, 6:08 p.m.
The attached patch optimizes
       __result_f = __builtin_powif (-1.0e+0, k);
to
   powi_cond_4 = k_1(D) & 1;
   powi_5 = powi_cond_4 ? -1.0e+0 : 1.0e+0;

I did an all-language (all,ada,go,objc++) bootstrap and regtest on 
x86-64-gnu-linux.
OK for the trunk?


Additionally, I would like to thank Thomas, who tried several approaches 
(e.g. fold_builtin_powi - which turned out to failed when called during 
optimization time, as regimplifing had problems with the COND_EXPR) and 
paved the way for this patch. And to Jakub, who gave helpful hints.

Note: Other replacements are also possible, e.g. "1.0 - (float) ((k & 1) 
<< 1)". However, the condition above turned out to be fasted as Jakub 
expected and Thomas has tested (see PR).


Tobias
Jeff Law - May 30, 2013, 6:27 p.m.
On 05/30/2013 12:08 PM, Tobias Burnus wrote:
> +		    }
> +		  else
> +		    {
> +		      if (!host_integerp (arg1, 0))
> +			break;
> +
> +		      n = TREE_INT_CST_LOW (arg1);
> +		      result = gimple_expand_builtin_powi (&gsi, loc, arg0, n);
> +		    }
In this case, why even bother with gimple_expand_builtin_powi.  Don't we 
know the result simply by looking at N and setting the dest to -1.0 or 
1.0 appropriately?

I don't see that powi_as_mults optimizes the case where both args are 
constants and thus the result is a trivially computable compile time 
constant.

Am I missing something?

Granted, I wouldn't expect it to happen often, but we might have started 
with a variable exponent and through various opts eventually collapsed 
it down to a known constant.


jeff
Tobias Burnus - May 30, 2013, 8:38 p.m.
Jeff Law wrote:
> On 05/30/2013 12:08 PM, Tobias Burnus wrote:
>> +            }
>> +          else
>> +            {
>> +              if (!host_integerp (arg1, 0))
>> +            break;
>> +
>> +              n = TREE_INT_CST_LOW (arg1);
>> +              result = gimple_expand_builtin_powi (&gsi, loc, arg0, n);
>> +            }
>
> In this case, why even bother with gimple_expand_builtin_powi. Don't 
> we know the result simply by looking at N and setting the dest to -1.0 
> or 1.0 appropriately?

I am a bit lost. The code quoted above is the old code - just moved down 
a bit. It is supposed to handle powi(x,n) for unknown x with known n - 
while the new code handles x == -1.0 for unknown n. Thus, 
gimple_expand_builtin_powi should be unreachable for x == -1.

My - possibly wrong - impression is that the case of n being constant 
*and* x being also a constant (e.g. x == -1) is handled before. One 
place where it is handled is at fold_builtin_powi. However, I do not 
understand GCC's passes well enough to know whether it is possible that 
one can reach tree-ssa-math-opts.c's execute_cse_sincos with x and n 
being becoming constant after the call to builtins.c's fold_builtin_powi.


Side remark: fold_builtin_powi would have been a more suitable case to 
do the conversion of this patch. However, re-gimplification does not 
like if one suddenly add an COND_EXPR at tree level, where the condition 
is neither a constant nor a variable. The problem is in the call to 
gimplify_modify_expr (from gimplify_cond_expr). If I recall correctly, 
for re-gimplification (but not for the initial gimplification), the code 
assumes that for "cond" is_gimple_val() is true. But as one has "x & 1", 
it is false - leading to an ICE. Thus, Richard + Jakub suggested to 
handle this case not on tree level during folding but on gimple level. 
That's how it ended up in tree-ssa-math-opts.c, where a special case for 
BUILT_IN_POWI already existed.

> I don't see that powi_as_mults optimizes the case where both args are 
> constants and thus the result is a trivially computable compile time 
> constant. Am I missing something?
> Granted, I wouldn't expect it to happen often, but we might have 
> started with a variable exponent and through various opts eventually 
> collapsed it down to a known constant.

If I understood it correctly, you would like to have an additional case 
before the newly added "k == 1", which does something like:

result = fold_builtin_powi (loc, NULL_TREE, arg1, arg2, TREE_TYPE (arg1);

if (result != NULL_TREE &&   .... /* Newly added  x == -1.0 case.  */

Is that what you propose?

Tobias
Jeff Law - May 30, 2013, 8:54 p.m.
On 05/30/2013 02:38 PM, Tobias Burnus wrote:
>
> I am a bit lost. The code quoted above is the old code - just moved down
> a bit. It is supposed to handle powi(x,n) for unknown x with known n -
> while the new code handles x == -1.0 for unknown n. Thus,
> gimple_expand_builtin_powi should be unreachable for x == -1.
Sorry, I misread the patch.    I was focused on the new lines and never 
looked back up to see if they were just copied from before.


>
> If I understood it correctly, you would like to have an additional case
> before the newly added "k == 1", which does something like:
>
> result = fold_builtin_powi (loc, NULL_TREE, arg1, arg2, TREE_TYPE (arg1);
>
> if (result != NULL_TREE &&   .... /* Newly added  x == -1.0 case.  */
>
> Is that what you propose?
Don't worry about it.    The patch is good as-is.



Jeff
Richard Guenther - May 31, 2013, 8:24 a.m.
On Thu, May 30, 2013 at 10:54 PM, Jeff Law <law@redhat.com> wrote:
> On 05/30/2013 02:38 PM, Tobias Burnus wrote:
>>
>>
>> I am a bit lost. The code quoted above is the old code - just moved down
>> a bit. It is supposed to handle powi(x,n) for unknown x with known n -
>> while the new code handles x == -1.0 for unknown n. Thus,
>> gimple_expand_builtin_powi should be unreachable for x == -1.
>
> Sorry, I misread the patch.    I was focused on the new lines and never
> looked back up to see if they were just copied from before.
>
>
>
>>
>> If I understood it correctly, you would like to have an additional case
>> before the newly added "k == 1", which does something like:
>>
>> result = fold_builtin_powi (loc, NULL_TREE, arg1, arg2, TREE_TYPE (arg1);
>>
>> if (result != NULL_TREE &&   .... /* Newly added  x == -1.0 case.  */
>>
>> Is that what you propose?
>
> Don't worry about it.    The patch is good as-is.

Why sink the !host_integerp check?  Please keep it where it is now.
Then

+                 if (real_minus_onep (arg0)
+                     && TREE_CODE (TREE_TYPE (arg1)) == INTEGER_TYPE

this check is redundant, too.

Richard.


>
>
> Jeff
>

Patch

2013-05-30  Tobias Brusnu  <burnus@net-b.de>

	PR middle-end/57073
	* tree-ssa-math-opts.c (execute_cse_sincos): Optimize
	powi (-1.0, k) to (k & 1) ? -1.0 : 1.0.

2013-05-30  Tobias Brusnu  <burnus@net-b.de>

	PR middle-end/57073
	* gfortran.dg/power_6.f90: New.

diff --git a/gcc/testsuite/gfortran.dg/power_6.f90 b/gcc/testsuite/gfortran.dg/power_6.f90
new file mode 100644
index 0000000..65d1bd0
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/power_6.f90
@@ -0,0 +1,15 @@ 
+! { dg-do compile }
+! { dg-options "-O1 -fdump-tree-optimized" }
+!
+! PR middle-end/57073
+! See also PR 57073
+!
+real function f(k)
+  integer, value :: k
+  f = (-1.0)**k
+end
+
+! { dg-final { scan-tree-dump-not "__builtin_powif"  "optimized" } }
+! { dg-final { scan-tree-dump "powi_cond_\[0-9\] = k_\[0-9\]\\(D\\) & 1;"  "optimized" } }
+! { dg-final { scan-tree-dump "powi_\[0-9\] = powi_cond_\[0-9\] \\? -1.0e\\+0 : 1.0e\\+0;"  "optimized" } }
+! { dg-final { cleanup-tree-dump "optimized" } }
diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c
index a94172d..a15c404 100644
--- a/gcc/tree-ssa-math-opts.c
+++ b/gcc/tree-ssa-math-opts.c
@@ -1445,12 +1445,43 @@  execute_cse_sincos (void)
 		CASE_FLT_FN (BUILT_IN_POWI):
 		  arg0 = gimple_call_arg (stmt, 0);
 		  arg1 = gimple_call_arg (stmt, 1);
-		  if (!host_integerp (arg1, 0))
-		    break;
-
-		  n = TREE_INT_CST_LOW (arg1);
 		  loc = gimple_location (stmt);
-		  result = gimple_expand_builtin_powi (&gsi, loc, arg0, n);
+
+		  if (real_minus_onep (arg0)
+		      && TREE_CODE (TREE_TYPE (arg1)) == INTEGER_TYPE
+		      && !host_integerp (arg1,0))
+		    {
+                      tree t0, t1, cond, one, minus_one;
+		      gimple stmt;
+
+		      t0 = TREE_TYPE (arg0);
+		      t1 = TREE_TYPE (arg1);
+		      one = build_real (t0, dconst1);
+		      minus_one = build_real (t0, dconstm1);
+
+		      cond = make_temp_ssa_name (t1, NULL, "powi_cond");
+		      stmt = gimple_build_assign_with_ops (BIT_AND_EXPR, cond,
+							   arg1,
+							   build_int_cst (t1,
+									  1));
+		      gimple_set_location (stmt, loc);
+		      gsi_insert_before (&gsi, stmt, GSI_SAME_STMT);
+
+		      result = make_temp_ssa_name (t0, NULL, "powi");
+		      stmt = gimple_build_assign_with_ops (COND_EXPR, result,
+							   cond,
+							   minus_one, one);
+		      gimple_set_location (stmt, loc);
+		      gsi_insert_before (&gsi, stmt, GSI_SAME_STMT);
+		    }
+		  else
+		    {
+		      if (!host_integerp (arg1, 0))
+			break;
+
+		      n = TREE_INT_CST_LOW (arg1);
+		      result = gimple_expand_builtin_powi (&gsi, loc, arg0, n);
+		    }
 
 		  if (result)
 		    {