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

["Carlos O'Donell" <carlos at redhat dot com>]

On 10/08/2014 08:48 PM, Joseph S. Myers wrote:
> 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).

I agree that other code in soft-fp is *much* more complicated, but 
this code is new and you have asked for review. We are not reviewing
the other code.

As I mentioned before there is a lack of reviewers precisely because
the code is difficult to review and understand even if you have read
the IEEE standard and understand the underlying binary formats.

>>> +#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 (...)

Almost.

Add a "/* Conditionals are organized to allow the compiler to optimize away
code based on the value of `ex`.  */" and I'm happy.

>> 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.

Sorry, yes, I said "indentation" but really meant: the lines changed in ways
unrelated to the patch and that makes review harder.

I have never used `git diff -w` and I will try it out.

Cheers,
Carlos.