Re: [PATCH v4 07/15] ARC: hardware floating point support

From: Joseph Myers <joseph@codesourcery.com>
To: Vineet Gupta <Vineet.Gupta1@synopsys.com>
Cc: linux-snps-arc@lists.infradead.org, libc-alpha@sourceware.org
Subject: Re: [PATCH v4 07/15] ARC: hardware floating point support
Date: Thu, 26 Mar 2020 23:22:51 +0000	[thread overview]
Message-ID: <alpine.DEB.2.21.2003262311330.24611@digraph.polyomino.org.uk> (raw)
In-Reply-To: <20200313030419.15843-8-vgupta@synopsys.com>

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
joseph@codesourcery.com

_______________________________________________
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc