diff mbox

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

Message ID 51A8BB8E.9050905@net-b.de
State New
Headers show

Commit Message

Tobias Burnus May 31, 2013, 3:02 p.m. UTC 
Am 31.05.2013 10:24, schrieb Richard Biener:
> On Thu, May 30, 2013 at 10:54 PM, Jeff Law <law@redhat.com> wrote:
>> Don't worry about it. The patch is good as-is. 
> Why sink the !host_integerp check?  Please keep it where it is now.
>

Answer: Because it doesn't work. And if I had a cup of coffee and didn't 
mess up my regtesting (by excluding the newly added test case), I had 
also seen that.

The very old code had (assume: powi(x,n)):
   if (n is not a constant)
     break;
   expand powi to "n" multiplications.

The original patch changed it to:

   if (n is a not constant and x == -1)
     result =  n & 1 ? -1.0 : 1.0
   else
     {
       if (n is not a constant)
         break;
       expand powi to "n" multiplications.
     }

Thus, if one moves up the condition
   if (n is not a constant)
     break;
the newly added code becomes unreachable.

However, I think the code is more readable if one simply removes the "&& 
!host_integerp (arg1,0)" from the x==1 case. Due to fold_builtin_powi 
having x==-1 and n == const should not happen - and if, the "n & 1 ? 
-1.0 : 1.0" is also not worse than an expanded multiplication (if n is 
large). [Alternatively, one can also keep (re-add) the "&& 
!host_integerp (arg1,0)".]

OK? (After successful bootstrap and regtesting.)

Tobias

Comments

Richard Biener June 3, 2013, 8:01 a.m. UTC | #1 
On Fri, May 31, 2013 at 5:02 PM, Tobias Burnus <burnus@net-b.de> wrote:
> Am 31.05.2013 10:24, schrieb Richard Biener:
>>
>> On Thu, May 30, 2013 at 10:54 PM, Jeff Law <law@redhat.com> wrote:
>>>
>>> Don't worry about it. The patch is good as-is.
>>
>> Why sink the !host_integerp check?  Please keep it where it is now.
>>
>
> Answer: Because it doesn't work. And if I had a cup of coffee and didn't
> mess up my regtesting (by excluding the newly added test case), I had also
> seen that.
>
> The very old code had (assume: powi(x,n)):
>   if (n is not a constant)
>     break;
>   expand powi to "n" multiplications.
>
> The original patch changed it to:
>
>   if (n is a not constant and x == -1)
>     result =  n & 1 ? -1.0 : 1.0
>   else
>     {
>       if (n is not a constant)
>         break;
>       expand powi to "n" multiplications.
>     }
>
> Thus, if one moves up the condition
>   if (n is not a constant)
>     break;
> the newly added code becomes unreachable.
>
> However, I think the code is more readable if one simply removes the "&&
> !host_integerp (arg1,0)" from the x==1 case. Due to fold_builtin_powi having
> x==-1 and n == const should not happen - and if, the "n & 1 ? -1.0 : 1.0" is
> also not worse than an expanded multiplication (if n is large).
> [Alternatively, one can also keep (re-add) the "&& !host_integerp
> (arg1,0)".]
>
> OK? (After successful bootstrap and regtesting.)

Ok.

Thanks,
Richard.

> Tobias
diff mbox

Patch

diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c
index b4de411..e9c32b3 100644
--- a/gcc/tree-ssa-math-opts.c
+++ b/gcc/tree-ssa-math-opts.c
@@ -1447,9 +1447,6 @@  execute_cse_sincos (void)
 		  arg1 = gimple_call_arg (stmt, 1);
 		  loc = gimple_location (stmt);
 
-		  if (!host_integerp (arg1, 0))
-		    break;
-
 		  if (real_minus_onep (arg0))
 		    {
                       tree t0, t1, cond, one, minus_one;
@@ -1477,6 +1474,9 @@  execute_cse_sincos (void)
 		    }
 		  else
 		    {
+		      if (!host_integerp (arg1, 0))
+			break;
+
 		      n = TREE_INT_CST_LOW (arg1);
 		      result = gimple_expand_builtin_powi (&gsi, loc, arg0, n);
 		    }