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

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

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

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

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

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
>