[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 23 Jan 2020 20:11:22 -0500
From: Rich Felker <dalias@...c.org>
To: musl@...ts.openwall.com
Subject: Re: Considering x86-64 fenv.s to C
On Fri, Jan 24, 2020 at 11:42:02AM +1100, Damian McGuckin wrote:
>
> In an attempt to address this issue, the following can be considered
> an attempt at a 'discussion paper'.
Thanks! I'm first commenting inline with the code since that's a more
natural way for me to review, but I might come back with some comments
on the text a bit later. Here it goes:
> #ifndef FE_ALL_EXCEPT
> #define FE_VALID_EXCEPT (FE_INEXACT|FE_DIV_BY_ZERO|FE_UNDERFLOW|FE_OVERFLOW)
> #define FE_ALL_EXCEPT (FE_INVALID|FE_VALID_EXCEPT)
> #endif
I don't understand what this is for. FE_ALL_EXCEPT is defined in the
public fenv.h and FE_VALID_EXCEPT is not used.
> /*
> * The usual approach to validating an exception mask is to throw out
> * anything which is illegal. Some systems will NOT do this, choosing
> * instead to return a non-zero result on encountering an illegal mask
> */
The latter is not desirable behavior and not something you need to be
trying to reproduce. It's a bug or at least an unspecified case we
don't care about in existing code.
> #ifndef FE_VALIDATE_EXCEPT
> #define FE_VALIDATE_EXCEPT(e) e &= FE_ALL_EXCEPT
> #endif
Write things like this directly in code; don't hide them behind
macros. Doing that just makes the code hard to read because you can't
tell what it's doing without searching for the macro definition. (Also
the macro is not protected by parens as written but that's easily
fixable.)
> /*
> * There is usually no need to mess the generic valid exceptions bits
> * given to 'feclearexcept' and 'feraiseexcept' so define a 'nothing'
> * macro for such a scenario - is this the MUSL accepted style????
> */
> #ifndef FE_QUALIFY_CLEAR_EXCEPT
> #define FE_QUALIFY_CLEAR_EXCEPT(excepts) (0)
> #endif
> #ifndef FE_QUALIFY_RAISE_EXCEPT
> #define FE_QUALIFY_RAISE_EXCEPT(excepts) (0)
> #endif
Can you clarify how these would be defined for powerpc? Could the
logic just be embedded in get_sr/set_sr if needed?
> /*
> * Compute the rounding mask with some rigid logic
> */
> #ifndef FE_TO_NEAREST
> #define FE_TO_NEAREST 0
> #define ROUND_MASK ((unsigned int) 0)
> #else
> #ifndef FE_TOWARDS_ZERO
> #define ROUND_MASK ((unsigned int) 0)
> #else
> #ifdef FE_DOWNWARD
> #ifdef FE_UPWARD
> #define ROUND_MASK ((unsigned int) (FE_DOWNWARD|FE_UPWARD|FE_TOWARDZERO))
> #else
> #define ROUND_MASK UNSUPPORTED rounding
> #endif
> #else
> #ifdef FE_UPWARD
> #define ROUND_MASK UNSUPPORTED rounding
> #else
> #define ROUND_MASK ((unsigned int) (FETOWARDZERO))
> #endif
> #endif
> #endif
> #endif
I don't see why you have "UNSUPPORTED rounding" cases here. Any
combination should be valid. Since these are not bitmasks, probably
the arch should just have to define the rounding mode mask. For
example if the values were 0b000, 0b001, 0b100, and 0b101, but the 0
in the middle was significant, it should probably be treated as part
of the mask.
If you really want to generate the mask, something like
#ifdef FE_TONEAREST
#define FE_TONEAREST_MASK FE_TONEAREST
#else
#define FE_TONEAREST_MASK 0
#endif
...
#define ROUND_MASK (FE_TONEAREST_MASK | FE_DOWNWARD_MASK | ...)
should be simpler and comprehensive.
> int
> feclearexcept(excepts)
> {
> FE_VALIDATE_EXCEPT(excepts);
> FE_QUALIFY_CLEAR_EXCEPT(excepts);
> setsr(getsr() & ~((unsigned int) excepts));
> return(0);
> }
No function-like parens around return value, no gratuitous casts.
> static inline int
> __raisearithmetically(int excepts)
> {
> /*
> * assume single OP is faster than double OP
> */
> const float one = (float) 1;
> const float zero = (float) 0;
> const float tiny = (float) 0x1.0p-126;
> const float huge = (float) 0x1.0p+126;
> volatile float x;
>
> /*
> * if it is just a simple exception, arithmetic expressions are optimal
> */
> switch(excepts)
> {
> case FE_INVALID:
> x = zero, x /= x;
> break;
> case FE_DIVBYZERO:
> x = zero, x = one / x;
> break;
> case FE_INEXACT:
> x = tiny, x += one;
> break;
> case (FE_OVERFLOW | FE_INEXACT):
> x = huge, x *= x;
> break;
> case (FE_UNDERFLOW | FE_INEXACT):
> x = tiny, x *= x;
> break;
> default: /* if more than one exception exists, a sledgehammer is viable */
> setsr(getsr() | ((unsigned int) excepts));
> break;
> }
> return(0);
> }
This is probably ok (aside from style things mentioned above), but
simply recording the flags in software without writing them to the
status register at all may be preferable. In that case we would not
even need a set_sr primitive, only get_sr and clear_sr.
Doing this would also simplify the ppc special-casing for FE_INVALID I
think..
> int
> feraiseexcept(int excepts)
> {
> FE_VALIDATE_EXCEPT(excepts);
> FE_QUALIFY_RAISE_EXCEPT(excepts);
> return __raisearithmetically(excepts);
> }
Then this could just be __pthread_self()->fpsr |= excepts;
> int
> fetestexcept(int excepts)
> {
> FE_VALIDATE_EXCEPT(excepts);
>
> return (int) (getsr() & ((unsigned int) excepts));
> }
> int
> fegetround(void)
> {
> return (int) (getcr() & ROUND_MASK);
> }
>
> int
> fesetround(int rounding_mode)
> {
> if ((rounding_mode & ~ROUND_MASK) == 0)
> {
> unsigned int mode = ((unsigned int) rounding_mode);
>
> return (setcr((getcr() & ~ROUND_MASK) | (mode & ROUND_MASK)), 0);
> }
> return(-1);
> }
I would put the error test at the beginning rather than putting the
whole body inside {}:
if (rounding_mode & ~ROUND_MASK) return -1;
> int
> fegetenv(fenv_t *envp)
> {
> return ((gete(envp)), 0);
> }
Is there a reason for using comma operator here?
> int
> fesetenv(fenv_t *envp)
> {
> fenv_t envpd = FE_DFL_ENV_DATA, *e = envp == FE_DFL_ENV ? &envpd : envp;
>
> return ((sete(envp)), 0);
> }
It might be preferable to use a static const object for default env;
I'm not sure. Not a big deal either way though.
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
