| Message ID | alpine.DEB.2.02.1204251548360.4560@laptop-mg.saclay.inria.fr |
|---|---|
| State | New |
| Headers | show |
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
--- 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; }