> On a high level, presumably there's no real value in keeping the old
> code to "fold" fpclassify. By exposing those operations as integer
> logicals for the fast path, if the FP value becomes a constant during
> the optimization pipeline we'll see the reinterpreted values flowing
> into the new integer logical tests and they'll simplify just like
> anything else. Right?
Yes, if it becomes a constant it will be folded away, both in the integer and
the fp case.
> The old IBM format is still supported, though they are expected to be
> moveing towards a standard ieee 128 bit format. So my only concern is
> that we preserve correct behavior for those cases -- I don't really care
> about optimizing them. So I think you need to keep them.
Yes, I re-added them. It's mostly a copy paste from what they were in the
other functions. But I have no way of testing it.
> For documenting builtins, using existing builtins as a template.
Yeah, I based them off the fpclassify documentation.
> > +{
> > + tree type = TREE_TYPE (arg);
> > +
> > + machine_mode mode = TYPE_MODE (type);
> > +
> > + const real_format *format = REAL_MODE_FORMAT (mode);
> > + const HOST_WIDE_INT type_width = TYPE_PRECISION (type);
> > + return (format->is_binary_ieee_compatible
> > + && FLOAT_WORDS_BIG_ENDIAN == WORDS_BIG_ENDIAN
> > + /* We explicitly disable quad float support on 32 bit systems. */
> > + && !(UNITS_PER_WORD == 4 && type_width == 128)
> > + && targetm.scalar_mode_supported_p (mode));
> > +}
> Presumably this is why you needed the target.h inclusion.
>
> Note that on some systems we even disable 64bit floating point support.
> I suspect this check needs a little re-thinking as I don't think that
> checking for a specific UNITS_PER_WORD is correct, nor is checking the
> width of the type. I'm not offhand sure what the test should be, just
> that I think we need something better here.
I think what I really wanted to test here is if there was an integer mode
available
which has the exact width as the floating point one. So I have replaced this
with
just a call to int_mode_for_mode. Which is probably more correct.
> > +
> > +/* Determines if the given number is a NaN value.
> > + This function is the last in the chain and only has to
> > + check if it's preconditions are true. */
> > +static tree
> > +is_nan (gimple_seq *seq, tree arg, location_t loc)
> So in the old code we checked UNGT_EXPR, in the new code's slow path you
> check UNORDERED. Was that change intentional?
The old FP code used UNORDERED and the new one was using ORDERED and negating
the result.
I've replaced it with UNORDERED, but both are correct.
Thanks for the review,
I'll get the new patch out ASAP.
Tamar
