This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH] Fixing improper conversion from sin() to sinf() in optimization mode.
- From: "Joseph S. Myers" <joseph at codesourcery dot com>
- To: Xinliang David Li <davidxl at google dot com>
- Cc: Cong Hou <congh at google dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 4 Sep 2013 22:26:09 +0000
- 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>
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
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-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