Re: [PATCH] Optimise the fpclassify builtin to perform integer operations when possible

[Andrew Pinski <pinskia@gmail.com>]

On Mon, Sep 12, 2016 at 6:21 PM, Moritz Klammler <moritz@klammler.eu> wrote:
>
> Tamar Christina <Tamar.Christina@arm.com> writes:
>
>> Hi All,
>>
>> This patch adds an optimized route to the fpclassify builtin
>> for floating point numbers which are similar to IEEE-754 in format.
>>
>> [...]
>
> I might be the least competent person on this list to review this patch
> but nevertheless read it out of interest and stumbled over a comment
> that I believe could be improved for clarity.
>
>     diff --git a/gcc/real.h b/gcc/real.h
>     index 59af580e78f2637be84f71b98b45ec6611053222..36ded57cf4db7c30c935bdb24219a167480f39c8 100644
>     --- a/gcc/real.h
>     +++ b/gcc/real.h
>     @@ -161,6 +161,15 @@ struct real_format
>        bool has_signed_zero;
>        bool qnan_msb_set;
>        bool canonical_nan_lsbs_set;
>     +
>     +  /* This flag indicates whether the format can be used in the optimized
>     +     code paths for the __builtin_fpclassify function and friends.
>     +     The format has to have the same NaN and INF representation as normal
>     +     IEEE floats (e.g. exp must have all bits set), most significant bit must be
>     +     sign bit, followed by exp bits of at most 32 bits.  Lastly the floating
>     +     point number must be representable as an integer.  The base of the number
>     +     also must be base 2.  */
>     +  bool is_binary_ieee_compatible;
>        const char *name;
>      };
>
> My first issue is that
>
>> The format has to have the same NaN and INF representation as normal
>> IEEE floats
>
> is kind of an oxymoron because NaNs and INFs are not "normal" IEEE
> floats.

Let me clarify here what was originally meant,  first some float uses
the same format as IEEE but don't support INF or NaNs (SPUv1 float for
an example, v2 supports both though).

Thanks,
Andrew.


>
> Second,
>
>> the floating point number must be representable as an integer
>
> is also somewhat misleading because it could be interpreted in the
> (obviously nonsensical) way that the floating-point *values* have to be
> integral.  (I think it should be possible to *interpret* not *represent*
> them as integers.)
>
> So I would like to suggest the following rewording.
>
>> This flag indicates whether the format is suitable for the optimized
>> code paths for the __builtin_fpclassify function and friends.  For
>> this, the format must be a base 2 representation with the sign bit as
>> the most-significant bit followed by (exp <= 32) exponent bits
>> followed by the mantissa bits.  It must be possible to interpret the
>> bits of the floating-point representation as an integer.  NaNs and
>> INFs must be represented by the same schema used by IEEE 754.  (NaNs
>> must be represented by an exponent with all bits 1, any mantissa
>> except all bits 0 and any sign bit.  +INF and -INF must be represented
>> by an exponent with all bits 1, a mantissa with all bits 0 and a sign
>> bit of 0 and 1 respectively.)
>
> I Hope this is clearer and still matches what the comment was supposed
> to say.
> --
> OpenPGP:
>
> Public Key:   http://openpgp.klammler.eu
> Fingerprint:  2732 DA32 C8D0 EEEC A081  BE9D CF6C 5166 F393 A9C0