Re: soft-fp: Support more precise "invalid" exceptions

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Wed, 8 Oct 2014, Carlos O'Donell wrote:

> On 09/18/2014 09:29 PM, Joseph S. Myers wrote:
> > Here is a corrected version of this patch with a bug in _FP_TO_INT's sNaN 
> > detection fixed.
> 
> Ignoring original patch and reviewing this one.
> 
> The changes in indentation make this patch annoying to review.
> 
> Your changes to _FP_CMP_CHECK_NAN are IMO too complicated
> to follow now.

_FP_CMP_CHECK_NAN is much simpler than most of the code in soft-fp, where 
typically you need to check for off-by-one errors in exponent 
manipulations that are written so that as much as possible gets folded to 
a single constant (without the compiler needing first to deduce that 
undefined integer overflow can't occur in a particular case).

> > +#define _FP_CMP_CHECK_NAN(fs, wc, X, Y, ex)				\
> > +  do									\
> > +    {									\
> > +      if (ex)								\
> > +	{								\
> > +	  if (FP_EX_INVALID_SNAN || FP_EX_INVALID_VC)			\
> > +	    {								\
> > +	      if ((ex) == 2)						\
> > +		FP_SET_EXCEPTION (FP_EX_INVALID | FP_EX_INVALID_VC);	\
> > +	      if (_FP_ISSIGNAN (fs, wc, X)				\
> > +		  || _FP_ISSIGNAN (fs, wc, Y))				\
> > +		FP_SET_EXCEPTION (FP_EX_INVALID | FP_EX_INVALID_SNAN);	\
> > +	    }								\
> > +	  else if ((ex) == 2						\
> > +		   || _FP_ISSIGNAN (fs, wc, X)				\
> > +		   || _FP_ISSIGNAN (fs, wc, Y))				\
> > +	    FP_SET_EXCEPTION (FP_EX_INVALID);				\
> > +	}								\
> 
> This is no longer OK for me. This is really hard to follow now.
> 
> This should really be broken out logically into a series of checks
> that we leave up to the compiler to reorganize. Unless you can show
> that such a source setup produces drastically worse code.

The broken-out case is the if (FP_EX_INVALID_SNAN || FP_EX_INVALID_VC) 
one - in that case, the two possible causes of exceptions are checked for 
separately.

That case would in fact be valid unconditionally (inside the "if (ex)", 
but with the (FP_EX_INVALID_SNAN || FP_EX_INVALID_VC) condition and the 
"else" case removed).  However, to avoid code generation changes to make 
the patch more obviously safe, and to make it obvious the checks for 
signaling NaNs will get optimized out when (ex) == 2 and the separate 
cases of "invalid" aren't being distinguished, the general broken-out case 
is separated from the common case where different cases of "invalid" 
exceptions aren't distinguished.

Perhaps more comments would help here?  Along the lines of

do {
/* The arguments are unordered, which may or may not result in an 
exception.  */
if (ex)
{
/* At least some cases of unordered arguments result in exceptions; check 
whether this is one.  */
if (FP_EX_INVALID_SNAN || FP_EX_INVALID_VC)
{
/* Check separately for each case of "invalid" exceptions.  */
}
/* Otherwise, we only need to check whether to raise an exception, not 
which case or cases it is.  */
else if (...)

> The change in indentation makes this hard to review.

It's not a change in indentation.  It's a move of the backslashes at end 
of line because some lines got longer and the backslashes are all meant to 
be aligned with each other.  In such cases it can be helpful to apply the 
patch then look at the changes with git diff -w.

-- 
Joseph S. Myers
joseph@codesourcery.com