Patchwork combine_conversions int->double->int

login
register
mail settings
Submitter Marc Glisse
Date April 25, 2012, 1:58 p.m.
Message ID <alpine.DEB.2.02.1204251548360.4560@laptop-mg.saclay.inria.fr>
Download mbox | patch
Permalink /patch/154930/
State New
Headers show

Comments

Marc Glisse - April 25, 2012, 1:58 p.m.
On Wed, 25 Apr 2012, Richard Guenther wrote:

> On Wed, Apr 25, 2012 at 10:12 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
>> Hello,
>>
>> a conversion like int->double->int is just the identity, as long as double
>> is big enough to represent all ints exactly. The most natural way I found to
>> do this optimization is the attached:
>>
>> 2012-04-25  Marc Glisse  <marc.glisse@inria.fr>
>>
>>        PR middle-end/27139
>>        * tree-ssa-forwprop.c (combine_conversions): Handle INT->FP->INT.
>>
>> Does the approach make sense? I don't know that code, and adding FLOAT_EXPR
>> / FIX_TRUNC_EXPR was a bit of guesswork. The precision of double could be
>> multiplied by log2(base), but not doing it is safe. If the approach is ok, I
>> could extend it so int->double->long also skips the intermediate conversion.
>>
>> Bootstrapped and regression tested on linux-x86_64.
>>
>> Should I try and write a testcase for a specific target checking for
>> specific asm instructions there, or is there a better way?
>
> Well, scanning the forwprop dump for example.
>
> Btw, I think not munging this new case into the existing CONVERT_EXPR_P
> code would be better - it makes the code (even) harder to understand and
> I'm not convinced that adding FLOAT_EXPR/FIX_TRUNC_EXPR handling
> does not wreck any assumptions in that code.
>
> It also seems that for DECIMAL_FLOAT_TYPE_P the transform is always
> valid?
>
> Richard.
>
>> (note that I can't commit)

Here is take 2 on this patch, which seems cleaner. Bootstrapped and 
regression tested.

gcc/ChangeLog
2012-04-25  Marc Glisse  <marc.glisse@inria.fr>

 	PR middle-end/27139
 	* tree-ssa-forwprop.c (combine_conversions): Handle INT->FP->INT.

gcc/testsuite/ChangeLog
2012-04-25  Marc Glisse  <marc.glisse@inria.fr>

 	PR middle-end/27139
 	* gcc.dg/tree-ssa/forwprop-18.c: New test.


In my patch, the lines with gimple_assign_* are vaguely guessed from what 
is around, I don't pretend to understand them.


While doing this, I noticed what I think is a mistake in a comment:
Richard Guenther - April 26, 2012, 1:30 p.m.
On Wed, Apr 25, 2012 at 3:58 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Wed, 25 Apr 2012, Richard Guenther wrote:
>
>> On Wed, Apr 25, 2012 at 10:12 AM, Marc Glisse <marc.glisse@inria.fr>
>> wrote:
>>>
>>> Hello,
>>>
>>> a conversion like int->double->int is just the identity, as long as
>>> double
>>> is big enough to represent all ints exactly. The most natural way I found
>>> to
>>> do this optimization is the attached:
>>>
>>> 2012-04-25  Marc Glisse  <marc.glisse@inria.fr>
>>>
>>>        PR middle-end/27139
>>>        * tree-ssa-forwprop.c (combine_conversions): Handle INT->FP->INT.
>>>
>>> Does the approach make sense? I don't know that code, and adding
>>> FLOAT_EXPR
>>> / FIX_TRUNC_EXPR was a bit of guesswork. The precision of double could be
>>> multiplied by log2(base), but not doing it is safe. If the approach is
>>> ok, I
>>> could extend it so int->double->long also skips the intermediate
>>> conversion.
>>>
>>> Bootstrapped and regression tested on linux-x86_64.
>>>
>>> Should I try and write a testcase for a specific target checking for
>>> specific asm instructions there, or is there a better way?
>>
>>
>> Well, scanning the forwprop dump for example.
>>
>> Btw, I think not munging this new case into the existing CONVERT_EXPR_P
>> code would be better - it makes the code (even) harder to understand and
>> I'm not convinced that adding FLOAT_EXPR/FIX_TRUNC_EXPR handling
>> does not wreck any assumptions in that code.
>>
>> It also seems that for DECIMAL_FLOAT_TYPE_P the transform is always
>> valid?
>>
>> Richard.
>>
>>> (note that I can't commit)
>
>
> Here is take 2 on this patch, which seems cleaner. Bootstrapped and
> regression tested.
>
> gcc/ChangeLog
>
> 2012-04-25  Marc Glisse  <marc.glisse@inria.fr>
>
>        PR middle-end/27139
>        * tree-ssa-forwprop.c (combine_conversions): Handle INT->FP->INT.
>
> gcc/testsuite/ChangeLog
>
> 2012-04-25  Marc Glisse  <marc.glisse@inria.fr>
>
>        PR middle-end/27139
>        * gcc.dg/tree-ssa/forwprop-18.c: New test.
>
>
> In my patch, the lines with gimple_assign_* are vaguely guessed from what is
> around, I don't pretend to understand them.

;)

The patch looks good to me - on a 2nd thought folding the case back in
to the if (CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (def_stmt)))
block to benefit from the local vars therein makes sense (but as separate
sub-if () as it is now).

Thanks,
Richard.

>
> While doing this, I noticed what I think is a mistake in a comment:
>
> --- gcc/real.c      (revision 186761)
> +++ gcc/real.c      (working copy)
> @@ -2814,11 +2814,11 @@ significand_size (enum machine_mode mode
>     return 0;
>
>   if (fmt->b == 10)
>     {
>       /* Return the size in bits of the largest binary value that can be
> -        held by the decimal coefficient for this mode.  This is one more
> +        held by the decimal coefficient for this mode.  This is one less
>         than the number of bits required to hold the largest coefficient
>         of this mode.  */
>       double log2_10 = 3.3219281;
>       return fmt->p * log2_10;
>     }
>
> --
> Marc Glisse

Patch

--- gcc/real.c      (revision 186761)
+++ gcc/real.c      (working copy)
@@ -2814,11 +2814,11 @@  significand_size (enum machine_mode mode
      return 0;

    if (fmt->b == 10)
      {
        /* Return the size in bits of the largest binary value that can be
-        held by the decimal coefficient for this mode.  This is one more
+        held by the decimal coefficient for this mode.  This is one less
          than the number of bits required to hold the largest coefficient
          of this mode.  */
        double log2_10 = 3.3219281;
        return fmt->p * log2_10;
      }