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*: Xinliang David Li <davidxl at google dot com>*To*: Cong Hou <congh 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 18:26:05 -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>

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) > + > + , 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. 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

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

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