This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc 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] PowerPC - Fix PPC64 floorl


On Thu, 2012-04-19 at 16:53 +0000, Joseph S. Myers wrote:
> On Tue, 17 Apr 2012, Adhemerval Zanella wrote:
> 
> > The commit 2460d3aa21f04cdf28497683bd3e29183189f779 added some additional tests for powl intended
> > to check a correction for i386/x86_64. The tests however started to trigger some issues with powl in
> > PPC64. I tracked the issue an it is due the floorl assembly version for PPC64 that fails to correctly
> > floor some values large than 2**52. 
> > 
> > This patch removes it (which is not an optimization any longer) and replace it by the C version 
> > adding also an optimization to values lower than 2**52.
> > 
> > --
> > 
> > 2012-04-17  Adhemerval Zanella  <azanella@linux.vnet.ibm.com>
> > 
> 
> [BZ #] number here.
> 
> > 	* sysdeps/ieee754/ldbl-128ibm/s_floorl.c: Add an optimization to values
> > 	lower than 2**52.
> > 	* sysdeps/powerpc/powerpc64/fpu/s_floorl.S: Delete file. Using the
> > 	sysdeps/ieee754/ldbl-128ibm/s_floorl.c version. 
> 
> Adding a self-contained testcase to the testsuite seems a good idea (even 
> though it's also covered by the pow tests).
> 
> > @@ -27,9 +27,13 @@ long double
> >  __floorl (long double x)
> >  {
> >    double xh, xl, hi, lo;
> > +  double two52 = 0x10000000000000LL;
> >  
> >    ldbl_unpack (x, &xh, &xl);
> >  
> > +  if (__builtin_expect(__builtin_fabs (x) < two52, 1))
> > +      return ldbl_pack (__floor (xh), -0.0);
> > +
> 
> I don't see how that can be correct.  If the high part is small, but an 
> integer, the result of this function should still depend on whether the 
> low part is positive, negative or zero.  Presumably the testsuite failed 
> to detect that bug in this patch, meaning you also need tests (conditioned 
> on TEST_LDOUBLE - they can be written to be valid for all long double 
> formats) for values that when rounded to 53 bits are an integer, but 
> differ from the integer value in lower bits (covering at least the four 
> cases of positive and negative value, above and below the exact integer it 
> is close to).

Are you concerned that xh may be "prerounded" away (from default
rounding mode)? This is not what we want for floor. In which case we
should "deround" xh before the __floor operation.
Perhaps:

+  if (__builtin_expect(__builtin_fabs (x) < two52, 1))
+      return ldbl_pack (__floor (xh+xl), -0.0);



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