Re: [PATCH v4 07/15] ARC: hardware floating point support
On 3/27/20 11:37 AM, Joseph Myers wrote: > feupdateenv has to preserve the previously raised exceptions even in the > FE_DFL_ENV case. It's equivalent to > > exc = fetestexcept (FE_ALL_EXCEPT); > fesetenv (envp); > feraiseexcept (exc); Ok. >> In some places I have following: >> >> if (((fpcr >> __FPU_RND_SHIFT) & FE_DOWNWARD) != round) >> >> So FE_DOWNWARD (0x3) is used as mask, is that OK or would you rather see >> >> #define __FPU_RND_MASK 0x3 > > I think it's cleanest to have a separate define for the mask. OK. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v4 07/15] ARC: hardware floating point support
On Fri, 27 Mar 2020, Vineet Gupta via Libc-alpha wrote: > > The bits to enable exception traps look like dynamic control mode bits to > > me. In general fegetmode should only need to mask off bits on > > architectures where the same register has both control and status bits, > > not on architectures where those are separate registers and fegetmode / > > fesetmode can work with the whole control register. > > Yeah, looking back into my old dev branch, that is how I did it initially, but > then switched to current implementation to "make get/set mode functions > inter-operate with get/set round" - although there was no inter-calling > between > the two. We can go back to that implementation as it seems slightly better in > generated code, but I'm curious if it is wrong too fegetmode / fesetmode deal with the complete set of dynamic control modes, not just rounding modes. I don't think any masking or shifting is needed or appropriate in fegetmode / fesetmode. > Is following pseudo-code correct for semantics ? > > fesetenv(env) > >if FE_DFL_ENV > fpcr = _FPU_DEFAULT; > fpsr = _FPU_FPSR_DEFAULT; >else > fpcr = envp->__fpcr; > fpsr = envp->__fpsr; > > feupdateenv(env) > >if FE_DFL_ENV > fpcr = _FPU_DEFAULT; > fpsr = _FPU_FPSR_DEFAULT; >else > fpcr = envp->__fpcr; > fpsr |= envp->__fpsr; <-- this is different feupdateenv has to preserve the previously raised exceptions even in the FE_DFL_ENV case. It's equivalent to exc = fetestexcept (FE_ALL_EXCEPT); fesetenv (envp); feraiseexcept (exc); > In some places I have following: > > if (((fpcr >> __FPU_RND_SHIFT) & FE_DOWNWARD) != round) > > So FE_DOWNWARD (0x3) is used as mask, is that OK or would you rather see > > #define __FPU_RND_MASK 0x3 I think it's cleanest to have a separate define for the mask. -- Joseph S. Myers jos...@codesourcery.com ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v4 07/15] ARC: hardware floating point support
On 3/26/20 4:22 PM, Joseph Myers wrote:
> On Thu, 12 Mar 2020, Vineet Gupta via Libc-alpha wrote:
>
>> +int
>> +fegetmode (femode_t *modep)
>> +{
>> + unsigned int fpcr;
>> +
>> + _FPU_GETCW (fpcr);
>> + *modep = fpcr >> __FPU_RND_SHIFT;
>
> The bits to enable exception traps look like dynamic control mode bits to
> me. In general fegetmode should only need to mask off bits on
> architectures where the same register has both control and status bits,
> not on architectures where those are separate registers and fegetmode /
> fesetmode can work with the whole control register.
Yeah, looking back into my old dev branch, that is how I did it initially, but
then switched to current implementation to "make get/set mode functions
inter-operate with get/set round" - although there was no inter-calling between
the two. We can go back to that implementation as it seems slightly better in
generated code, but I'm curious if it is wrong too
>> +int
>> +__fesetround (int round)
>> +{
>> + unsigned int fpcr;
>> +
>> + _FPU_GETCW (fpcr);
>> +
>> + if (__glibc_unlikely (((fpcr >> __FPU_RND_SHIFT) & FE_DOWNWARD) != round))
>> +{
>> + fpcr = (fpcr & ~(FE_DOWNWARD << __FPU_RND_SHIFT)) | (round <<
>> __FPU_RND_SHIFT);
>> + _FPU_SETCW (fpcr);
>> +}
>
> I don't think the use of __glibc_unlikely is appropriate here. It's not
> at all clear to me that the normal fesetround case is setting the rounding
> mode to the value it already has, as the use of __glibc_unlikely would
> suggest.
Ok removed.
>> +int
>> +__feupdateenv (const fenv_t *envp)
>> +{
>> + unsigned int fpcr;
>> + unsigned int fpsr;
>> +
>> + _FPU_GETCW (fpcr);
>> + _FPU_GETS (fpsr);
>> +
>> + /* rounding mode set to what is in env. */
>> + fpcr = envp->__fpcr;
>> +
>> + /* currently raised exceptions are OR'ed with env. */
>> + fpsr |= envp->__fpsr;
>
> This looks like it wouldn't work for FE_DFL_ENV, which is a valid argument
> to feupdateenv.
Is following pseudo-code correct for semantics ?
fesetenv(env)
if FE_DFL_ENV
fpcr = _FPU_DEFAULT;
fpsr = _FPU_FPSR_DEFAULT;
else
fpcr = envp->__fpcr;
fpsr = envp->__fpsr;
feupdateenv(env)
if FE_DFL_ENV
fpcr = _FPU_DEFAULT;
fpsr = _FPU_FPSR_DEFAULT;
else
fpcr = envp->__fpcr;
fpsr |= envp->__fpsr; <-- this is different
> It looks like we're missing test coverage for feupdateenv
> (FE_DFL_ENV) (we have coverage for feupdateenv (FE_NOMASK_ENV) and
> fesetenv (FE_DFL_ENV)).
>
>> +static inline int
>> +get_rounding_mode (void)
>> +{
>> +#if defined(__ARC_FPU_SP__) || defined(__ARC_FPU_DP__)
>> + unsigned int fpcr;
>> + _FPU_GETCW (fpcr);
>> +
>> + return fpcr >> __FPU_RND_SHIFT;
>
> Both here and in fegetround you're not doing anything to mask off high
> bits of the control register. That seems unsafe to me, should future
> processors add new control bits in the high bits that might sometimes be
> nonzero.
Yeah we can certainly add masking for future proofing.
In some places I have following:
if (((fpcr >> __FPU_RND_SHIFT) & FE_DOWNWARD) != round)
So FE_DOWNWARD (0x3) is used as mask, is that OK or would you rather see
#define __FPU_RND_MASK 0x3
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v4 07/15] ARC: hardware floating point support
On Thu, 12 Mar 2020, Vineet Gupta via Libc-alpha wrote:
> +int
> +fegetmode (femode_t *modep)
> +{
> + unsigned int fpcr;
> +
> + _FPU_GETCW (fpcr);
> + *modep = fpcr >> __FPU_RND_SHIFT;
The bits to enable exception traps look like dynamic control mode bits to
me. In general fegetmode should only need to mask off bits on
architectures where the same register has both control and status bits,
not on architectures where those are separate registers and fegetmode /
fesetmode can work with the whole control register.
> +int
> +__fesetround (int round)
> +{
> + unsigned int fpcr;
> +
> + _FPU_GETCW (fpcr);
> +
> + if (__glibc_unlikely (((fpcr >> __FPU_RND_SHIFT) & FE_DOWNWARD) != round))
> +{
> + fpcr = (fpcr & ~(FE_DOWNWARD << __FPU_RND_SHIFT)) | (round <<
> __FPU_RND_SHIFT);
> + _FPU_SETCW (fpcr);
> +}
I don't think the use of __glibc_unlikely is appropriate here. It's not
at all clear to me that the normal fesetround case is setting the rounding
mode to the value it already has, as the use of __glibc_unlikely would
suggest.
> +int
> +__feupdateenv (const fenv_t *envp)
> +{
> + unsigned int fpcr;
> + unsigned int fpsr;
> +
> + _FPU_GETCW (fpcr);
> + _FPU_GETS (fpsr);
> +
> + /* rounding mode set to what is in env. */
> + fpcr = envp->__fpcr;
> +
> + /* currently raised exceptions are OR'ed with env. */
> + fpsr |= envp->__fpsr;
This looks like it wouldn't work for FE_DFL_ENV, which is a valid argument
to feupdateenv. It looks like we're missing test coverage for feupdateenv
(FE_DFL_ENV) (we have coverage for feupdateenv (FE_NOMASK_ENV) and
fesetenv (FE_DFL_ENV)).
> +static inline int
> +get_rounding_mode (void)
> +{
> +#if defined(__ARC_FPU_SP__) || defined(__ARC_FPU_DP__)
> + unsigned int fpcr;
> + _FPU_GETCW (fpcr);
> +
> + return fpcr >> __FPU_RND_SHIFT;
Both here and in fegetround you're not doing anything to mask off high
bits of the control register. That seems unsafe to me, should future
processors add new control bits in the high bits that might sometimes be
nonzero.
--
Joseph S. Myers
jos...@codesourcery.com
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v4 07/15] ARC: hardware floating point support
On 3/25/20 7:06 PM, Joseph Myers wrote: > On Thu, 12 Mar 2020, Vineet Gupta via Libc-alpha wrote: > >> diff --git a/sysdeps/arc/bits/fenv.h b/sysdeps/arc/bits/fenv.h > > This is another example of one patch fixing up another. You're using the > same ABI for both hard and soft float, so the first patch adding a > bits/fenv.h header should be using that ABI for fenv_t, rather than one > patch adding it with one ABI then a subsequent patch changing the > definition of that type. Sorry about that. Indeed its a different mindset how we things do in glibc and I'm learning that slowly. I've fixed soft-float so this file is not touched anymore for hard-float. >> diff --git a/sysdeps/arc/fpu/libm-test-ulps b/sysdeps/arc/fpu/libm-test-ulps > > This will need updating for the recent changes to remove separate inline > function testing (so there should be no ifloat or idouble entries any > more). OK. FWIW I'm counting on Alistair's 64-bit stuff (and his frequent rebases) to be able to pick up latest upstream and then do the adjustments in ARC port. His changes seem rock solid in my ARC testing and I was hoping they get merged sooner. >> diff --git a/sysdeps/arc/tininess.h b/sysdeps/arc/tininess.h >> new file mode 100644 >> index ..1db37790f881 >> --- /dev/null >> +++ b/sysdeps/arc/tininess.h >> @@ -0,0 +1 @@ >> +#define TININESS_AFTER_ROUNDING 1 > > In the soft-float patch you define _FP_TININESS_AFTER_ROUNDING to 0. > Formally it doesn't really matter since you aren't supporting exceptions > for soft-float anyway. But typically I'd expect the definition of > _FP_TININESS_AFTER_ROUNDING, on architecture with support for both hard > and soft float, to match the architecture's rule for tininess detection > for hard-float. OK I've updated soft-float. Thx, -Vineet ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: [PATCH v4 07/15] ARC: hardware floating point support
On Thu, 12 Mar 2020, Vineet Gupta via Libc-alpha wrote: > diff --git a/sysdeps/arc/bits/fenv.h b/sysdeps/arc/bits/fenv.h This is another example of one patch fixing up another. You're using the same ABI for both hard and soft float, so the first patch adding a bits/fenv.h header should be using that ABI for fenv_t, rather than one patch adding it with one ABI then a subsequent patch changing the definition of that type. > diff --git a/sysdeps/arc/fpu/libm-test-ulps b/sysdeps/arc/fpu/libm-test-ulps This will need updating for the recent changes to remove separate inline function testing (so there should be no ifloat or idouble entries any more). > diff --git a/sysdeps/arc/tininess.h b/sysdeps/arc/tininess.h > new file mode 100644 > index ..1db37790f881 > --- /dev/null > +++ b/sysdeps/arc/tininess.h > @@ -0,0 +1 @@ > +#define TININESS_AFTER_ROUNDING 1 In the soft-float patch you define _FP_TININESS_AFTER_ROUNDING to 0. Formally it doesn't really matter since you aren't supporting exceptions for soft-float anyway. But typically I'd expect the definition of _FP_TININESS_AFTER_ROUNDING, on architecture with support for both hard and soft float, to match the architecture's rule for tininess detection for hard-float. -- Joseph S. Myers jos...@codesourcery.com ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
