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

[Moritz Klammler <moritz@klammler.eu>]

[-- Attachment #1.1: Type: text/plain, Size: 2858 bytes --]


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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 454 bytes --]