This is the mail archive of the
`gcc-patches@gcc.gnu.org`
mailing list for the GCC project.

Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|

Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |

Other format: | [Raw text] |

*From*: Cong Hou <congh at google dot com>*To*: Xinliang David Li <davidxl at google dot com>*Cc*: "Joseph S. Myers" <joseph at codesourcery dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>*Date*: Mon, 9 Sep 2013 19:28:55 -0700*Subject*: Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.*Authentication-results*: sourceware.org; auth=none*References*: <CAK=A3=1b=qhx8u8Wz7je=KYUbvOQHyKaWP353ud7D7f8gF56Bw at mail dot gmail dot com> <CAK=A3=3=gLhTso3+AF-BmiONPsEpP3dGTFtOAZPbh+oteYPTNA at mail dot gmail dot com> <Pine dot LNX dot 4 dot 64 dot 1308302148230 dot 22363 at digraph dot polyomino dot org dot uk> <CAK=A3=0bQkcvprFZTtuJ0ZNbknSJixhMP559tiF3FFUL0zkmfw at mail dot gmail dot com> <Pine dot LNX dot 4 dot 64 dot 1308311615070 dot 20398 at digraph dot polyomino dot org dot uk> <CAK=A3=2PQh5RiuDWn9yGv-jxkC5G-s2JRSQTF0PEmaQSpsnyZg at mail dot gmail dot com> <Pine dot LNX dot 4 dot 64 dot 1309032123250 dot 27960 at digraph dot polyomino dot org dot uk> <CAAkRFZK1wL00=eU02LqEMVEpSxxgn8dFHhJEy6XbpYkB1T+X6g at mail dot gmail dot com> <Pine dot LNX dot 4 dot 64 dot 1309032137370 dot 27960 at digraph dot polyomino dot org dot uk> <CAK=A3=35nZpFVe=Y1J1mnWA==CtiMSifwBrvPmDt7G0c8Pz0AQ at mail dot gmail dot com> <Pine dot LNX dot 4 dot 64 dot 1309032232360 dot 27960 at digraph dot polyomino dot org dot uk> <CAK=A3=1m5Q1gAe0Ga+kQc9Cmaf1yoL7XXgi0GKB+7msvfznQrg at mail dot gmail dot com> <Pine dot LNX dot 4 dot 64 dot 1309042057520 dot 16060 at digraph dot polyomino dot org dot uk> <CAAkRFZKPxgjwHdM34kk2yOsXsVzqWHzfPGcaoViQyshGcLwfJw at mail dot gmail dot com> <Pine dot LNX dot 4 dot 64 dot 1309042141500 dot 16060 at digraph dot polyomino dot org dot uk> <CAK=A3=2jYZBkyhHM=5__s6DCqKv1Af_PmOHZLANpj1OOXoUEPA at mail dot gmail dot com> <CAAkRFZ+o4LLA=RzBtEVxFO4mEC4tbcSUCxvLPrEEzb++qP6=eg at mail dot gmail dot com>

On Mon, Sep 9, 2013 at 6:26 PM, Xinliang David Li <davidxl@google.com> wrote: > On Fri, Sep 6, 2013 at 3:24 PM, Cong Hou <congh@google.com> wrote: >> First, thank you for your detailed comments again! Then I deeply >> apologize for not explaining my patch properly and responding to your >> previous comment. I didn't understand thoroughly the problem before >> submitting the patch. >> >> Previously I only considered the following three conversions for sqrt(): >> >> >> 1: (float) sqrt ((double) float_val) -> sqrtf (float_val) >> 2: (float) sqrtl ((long double) float_val) -> sqrtf (float_val) >> 3: (double) sqrtl ((long double) double_val) -> sqrt (double_val) >> >> >> We have four types here: >> >> TYPE is the type to which the result of the function call is converted. >> ITYPE is the type of the math call expression. >> TREE_TYPE(arg0) is the type of the function argument (before type conversion). >> NEWTYPE is chosen from TYPE and TREE_TYPE(arg0) with higher precision. >> It will be the type of the new math call expression after conversion. >> >> For all three cases above, TYPE is always the same as NEWTYPE. That is >> why I only considered TYPE during the precision comparison. ITYPE can >> only be double_type_node or long_double_type_node depending on the >> type of the math function. That is why I explicitly used those two >> types instead of ITYPE (no correctness issue). But you are right, >> ITYPE is more elegant and better here. >> >> After further analysis, I found I missed two more cases. Note that we >> have the following conditions according to the code in convert.c: >> >> TYPE_PRECISION(NEWTYPE) >= TYPE_PRECISION(TYPE) >> TYPE_PRECISION(NEWTYPE) >= TYPE_PRECISION(TREE_TYPE(arg0)) >> TYPE_PRECISION (NEWTYPE) < TYPE_PRECISION (ITYPE) >> >> the last condition comes from the fact that we only consider >> converting a math function call into another one with narrower type. >> Therefore we have >> >> TYPE_PRECISION(TYPE) < TYPE_PRECISION (ITYPE) >> TYPE_PRECISION(TREE_TYPE(arg0)) < TYPE_PRECISION (ITYPE) >> >> So for sqrt(), TYPE and TREE_TYPE(arg0) can only be float, and for >> sqrtl(), TYPE and TREE_TYPE(arg0) can be either float or double with >> four possible combinations. Therefore we have two more conversions to >> consider besides the three ones I mentioned above: >> >> >> 4: (float) sqrtl ((long double) double_val) -> (float) sqrt (double_val) >> 5: (double) sqrtl ((long double) float_val) -> sqrt ((double) float_val) >> >> >> For the first conversion here, TYPE (float) is different from NEWTYPE >> (double), and my previous patch doesn't handle this case.The correct >> way is to compare precisions of ITYPE and NEWTYPE now. >> >> To sum up, we are converting the expression >> >> (TYPE) sqrtITYPE ((ITYPE) expr) >> >> to >> >> (TYPE) sqrtNEWTYPE ((NEWTYPE) expr) >> >> and we require >> >> PRECISION (ITYPE) >= PRECISION (NEWTYPE) * 2 + 2 >> >> to make it a safe conversion. >> >> >> The new patch is pasted below. >> >> I appreciate your detailed comments and analysis, and next time when I >> submit a patch I will be more carefully about the reviewer's comment. >> >> >> Thank you! >> >> Cong >> >> >> >> Index: gcc/convert.c >> =================================================================== >> --- gcc/convert.c (revision 201891) >> +++ gcc/convert.c (working copy) >> @@ -135,16 +135,19 @@ convert_to_real (tree type, tree expr) >> CASE_MATHFN (COS) >> CASE_MATHFN (ERF) >> CASE_MATHFN (ERFC) >> - CASE_MATHFN (FABS) >> CASE_MATHFN (LOG) >> CASE_MATHFN (LOG10) >> CASE_MATHFN (LOG2) >> CASE_MATHFN (LOG1P) >> - CASE_MATHFN (LOGB) >> CASE_MATHFN (SIN) >> - CASE_MATHFN (SQRT) >> CASE_MATHFN (TAN) >> CASE_MATHFN (TANH) >> + /* The above functions are not safe to do this conversion. */ >> + if (!flag_unsafe_math_optimizations) >> + break; >> + CASE_MATHFN (SQRT) >> + CASE_MATHFN (FABS) >> + CASE_MATHFN (LOGB) >> #undef CASE_MATHFN >> { >> tree arg0 = strip_float_extensions (CALL_EXPR_ARG (expr, 0)); >> @@ -155,6 +158,27 @@ convert_to_real (tree type, tree expr) >> if (TYPE_PRECISION (TREE_TYPE (arg0)) > TYPE_PRECISION (type)) >> newtype = TREE_TYPE (arg0); >> >> + /* We consider to convert >> + >> + (T1) sqrtT2 ((T2) exprT3) >> + to >> + (T1) sqrtT4 ((T4) exprT3) > > Should this be > > (T4) sqrtT4 ((T4) exprT3) T4 is not necessarily the same as T1. For the conversion: (float) sqrtl ((long double) double_val) -> (float) sqrt (double_val) T4 is double and T1 is float. >> + >> + , where T1 is TYPE, T2 is ITYPE, T3 is TREE_TYPE (ARG0), >> + and T4 is NEWTYPE. > > NEWTYPE is also the wider one between T1 and T3. Right. Actually this is easy to catch from the context just before this comment. tree newtype = type; if (TYPE_PRECISION (TREE_TYPE (arg0)) > TYPE_PRECISION (type)) newtype = TREE_TYPE (arg0); thanks, Cong > > David > >> All those types are of floating point types. >> + T4 (NEWTYPE) should be narrower than T2 (ITYPE). This conversion >> + is safe only if P1 >= P2*2+2, where P1 and P2 are precisions of >> + T2 and T4. See the following URL for a reference: >> + http://stackoverflow.com/questions/9235456/determining-floating-point-square-root >> + */ >> + if (fcode == BUILT_IN_SQRT || fcode == BUILT_IN_SQRTL) >> + { >> + int p1 = REAL_MODE_FORMAT (TYPE_MODE (itype))->p; >> + int p2 = REAL_MODE_FORMAT (TYPE_MODE (newtype))->p; >> + if (p1 < p2 * 2 + 2 && !flag_unsafe_math_optimizations) >> + break; >> + } >> + >> /* Be careful about integer to fp conversions. >> These may overflow still. */ >> if (FLOAT_TYPE_P (TREE_TYPE (arg0)) >> Index: gcc/testsuite/gcc.c-torture/execute/20030125-1.c >> =================================================================== >> --- gcc/testsuite/gcc.c-torture/execute/20030125-1.c (revision 201891) >> +++ gcc/testsuite/gcc.c-torture/execute/20030125-1.c (working copy) >> @@ -44,11 +44,11 @@ __attribute__ ((noinline)) >> double >> sin(double a) >> { >> - abort (); >> + return a; >> } >> __attribute__ ((noinline)) >> float >> sinf(float a) >> { >> - return a; >> + abort (); >> } >> >> On Wed, Sep 4, 2013 at 3:26 PM, Joseph S. Myers <joseph@codesourcery.com> wrote: >>> On Wed, 4 Sep 2013, Xinliang David Li wrote: >>> >>>> > This patch submission still fails to pay attention to various of my >>>> > comments. >>>> > >>>> >>>> If you can provide inlined comments in the patch, that will be more >>>> useful and productive. >>> >>> I have explained things several times in this thread already. I see no >>> point in repeating things when what I would say has already been said and >>> ignored. As far as I can tell, this latest patch submission has taken one >>> line from the message it is in response to, and largely ignored the >>> following two paragraphs (including where I explicitly say that said line >>> should not appear literally in the source code at all). But, repeating >>> what I said before yet again: >>> >>> (but you should be referring to the relevant types >>> >>> The patch does not do properly that. It refers explicitly to >>> long_double_type_node and double_type_node. >>> >>> - "type", the type >>> being converted to, "itype", the type of the function being called in the >>> source code, "TREE_TYPE (arg0)", the type of the argument after extensions >>> have been removed, and "newtype", computed from those >>> >>> The patch only engages with "type". I suspect "newtype" is what it really >>> means there when using "type". When it uses long_double_type_node and >>> double_type_node, those should be "itype". >>> >>> - so you should have >>> expressions like the above with two or more of those four types, but not >>> with long_double_type_node directly). >>> >>> See above. The patch uses long_double_type_node directly, contrary to >>> what I explicitly said. You are free to disagree with something I say in >>> a review - but in that case you need to reply specifically to the review >>> comment explaining your rationale for disagreeing with it. Just ignoring >>> the comment and not mentioning the disagreement wastes the time of >>> reviewers. >>> >>> The patch submission will need to include a proper analysis to justify to >>> the reader why the particular inequality with particular types from those >>> four is correct in all cases where the relevant code may be executed. >>> >>> The comments only refer to "T1" and "T2" without explaining how they >>> relate to the original expression (three types) or the variables within >>> GCC dealing with various types (four types, because newtype gets >>> involved). I said back in >>> <http://gcc.gnu.org/ml/gcc-patches/2013-09/msg00161.html> and >>> <http://gcc.gnu.org/ml/gcc-patches/2013-08/msg01384.html> that three types >>> are involved - when I say "the patch submission needs to include its own >>> analysis for the full generality of three types", again, it's >>> inappropriate for a patch to omit such an analysis without justification. >>> The patch submission needs to include an analysis that properly explains >>> the transformation involved and the conditions under which it is safe. >>> Maybe starting along the lines of: >>> >>> We have an expression of the form (T1) sqrtT2 ((T2) exprT3), where exprT3 >>> has type T3 (TREE_TYPE (ARG0)), T2 is the type of the floating-point >>> square root function being used (ITYPE), T1 is TYPE and all these types >>> are binary floating-point types. We wish to optimize if possible to an >>> expression of the form (T1) sqrtT4 ((T4) exprT3), where T4 (NEWTYPE) is >>> narrower than T2. (Then explain the choice of T4 and the conditions under >>> which the transformation is safe, with appropriate references.) >>> >>> I suggest that for the next patch submission you (the patch submitter) >>> make sure you spend at least an hour properly understanding the issues and >>> all the previous messages in the thread and writing up the detailed, >>> coherent explanation of the transformation and analysis of the issues that >>> I asked for some time ago, as if for a reader who hasn't read any of this >>> thread or looked at this transformation in GCC before. I've certainly >>> spent longer than that on review in this thread. It's normal for a patch >>> involving anything at all tricky to take hours to write up (and also >>> normal for one to discover, in the course of writing the detailed coherent >>> analysis for people who aren't familiar with the code and the issues >>> involved, that there's in fact something wrong with the patch and it needs >>> revisiting before submission). >>> >>> -- >>> Joseph S. Myers >>> joseph@codesourcery.com

**Follow-Ups**:

**References**:**Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.***From:*Cong Hou

**Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.***From:*Joseph S. Myers

**Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.***From:*Xinliang David Li

**Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.***From:*Joseph S. Myers

**Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.***From:*Cong Hou

**Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.***From:*Joseph S. Myers

**Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.***From:*Cong Hou

**Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.***From:*Joseph S. Myers

**Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.***From:*Xinliang David Li

**Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.***From:*Joseph S. Myers

**Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.***From:*Cong Hou

**Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.***From:*Xinliang David Li

Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|

Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |