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]

Re: [PATCH, PR59833]: Fix sNaN handling in ARM float to double conversion


On 2016-07-20 11:22, Ramana Radhakrishnan wrote:
> On Wed, Jul 20, 2016 at 11:14 AM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> > On 2016-07-20 11:04, Ramana Radhakrishnan wrote:
> >> On Wed, Jul 20, 2016 at 10:56 AM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> >> > On 2016-07-20 10:10, Ramana Radhakrishnan wrote:
> >> >> On Wed, Jul 20, 2016 at 8:48 AM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> >> >> > On ARM soft-float, the float to double conversion doesn't convert a sNaN
> >> >> > to qNaN as the IEEE Std 754 standard mandates:
> >> >> >
> >> >> > "Under default exception handling, any operation signaling an invalid
> >> >> > operation exception and for which a floating-point result is to be
> >> >> > delivered shall deliver a quiet NaN."
> >> >> >
> >> >> > Given the soft float ARM code ignores exceptions and always provides a
> >> >> > result, a float to double conversion of a signaling NaN should return a
> >> >> > quiet NaN. Fix this in extendsfdf2.
> >> >> >
> >> >> > gcc/ChangeLog:
> >> >> >
> >> >> >         PR target/59833
> >> >> >         * config/arm/ieee754-df.S (extendsfdf2): Convert sNaN to qNaN.
> >> >> >
> >> >> > gcc/testsuite/ChangeLog:
> >> >> >
> >> >> >         * gcc.dg/pr59833.c: New testcase.
> >> >>
> >> >>
> >> >> Ok - assuming this was tested appropriately with no regressions.
> >> >
> >> > Given it only touches arm code, I only tested it on arm and I have seen
> >> > no regression. That said I wouldn't be surprised if the new testcase
> >> > fails on some other architectures.
> >>
> >> I was assuming you tested it on ARM :)  In this case given the change
> >> is only in the backend I would have expected this patch to have been
> >> tested for soft-float ARM or an appropriate multilib. Saying what
> >> configuration the patch was tested on is useful for the audit trail.
> >> For e.g. it's no use testing this patch on armhf ( i.e.
> >> --with-float=hard --with-fpu=vfpv3/neon --with-arch=armv7-a) as by
> >> default the test would never generate the call to the library function
> >> but I'm sure you know all that anyway.
> >
> > Indeed I should have given more details. I tested it on a Debian armel
> > machine, and I configured GCC the same way as the Debian package, that
> > is using --with-arch=armv4t --with-float=soft.
> >
> > I built it once with the new test but without the fix and a second time
> > with both the test and the fix. I have verified that the test fails in
> > the first case and pass in the second case.
> 
> Thanks for the info - what about all the other regression tests ? Did
> you do a full make check and ensure that no other tests regressed in
> comparison ?  Patches need to be tested against the entire regression
> testsuite and not just what was added.

Yes, I compared the testsuite result between the two runs, and there are
identical beside this new test (hence my "I have seen no regression" in
my first answer).

Aurelien

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
aurelien@aurel32.net                 http://www.aurel32.net


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]