Bug in builtin_floor optimization

Bug in builtin_floor optimization

[Dale Johannesen]

[-- Attachment #1: Type: text/plain, Size: 301 bytes --]

There is some clever code in convert_to_real that converts

double d;
(float)floor(d)

to

floorf((float)d)

(on targets where floor and floorf are considered builtins.)
This is wrong, because the (float)d conversion normally uses
round-to-nearest and can round up to the next integer.  For example:


[-- Attachment #2: 4221664b.c --]
[-- Type: text/plain, Size: 305 bytes --]

double d = 1024.0 - 1.0 / 32768.0;
extern double floor(double);
extern float floorf(float);
extern int printf(const char*, ...);

int main() {

    double df = floor(d);
    float f1 = (float)floor(d);

    printf("floor(%f) = %f\n", d, df);
    printf("(float)floor(%f) = %f\n", d, f1);

    return 0;
}

[-- Attachment #3: Type: text/plain, Size: 295 bytes --]



with -O2.
The transformation is also done for ceil, round, rint, trunc and 
nearbyint.
I'm not a math guru, but it looks like ceil, rint, trunc and nearbyint 
are
also unsafe for this transformation.  round may be salvageable.
Comments?  Should I preserve the buggy behavior with -ffast-math?

Re: Bug in builtin_floor optimization

[Roger Sayle]

On Mon, 22 Aug 2005, Dale Johannesen wrote:
> There is some clever code in convert_to_real that converts
>
> double d;
> (float)floor(d)
>
> to
>
> floorf((float)d)
> ...
>
> Comments? Should I preserve the buggy behavior with -ffast-math?

Good catch.  This is indeed a -ffast-math (or more precisely a
flag_unsafe_math_optimizations) transformation.  I'd prefer to
keep these transformations with -ffast-math, as Jan described them
as significantly helping SPEC's mesa when they were added.

My one comment is that we should try to make sure that we continue
to optimize the common safe case (even without -ffast-math):

	float x, y;
	x = floor(y);

i.e. that (float)floor((double)y) is the same as floorf(y).
Hmm, it might be good to know the relative merits of the safe vs.
unsafe variants.  If the majority of the benefit is from the "safe"
form, I wouldn't be opposed to removing the "unsafe" form completely,
if people think its an optimization "too far".

Thanks for investigating this.

Roger
--

Re: Bug in builtin_floor optimization

[Richard Henderson]

On Tue, Aug 23, 2005 at 09:28:50AM -0600, Roger Sayle wrote:
> Good catch.  This is indeed a -ffast-math (or more precisely a
> flag_unsafe_math_optimizations) transformation.  I'd prefer to
> keep these transformations with -ffast-math, as Jan described them
> as significantly helping SPEC's mesa when they were added.

Are you sure it was "(float)floor(d)"->"floorf((float)d)" that
helped mesa and not "(float)floor((double)f)"->"floorf(f)" ?

It wouldn't bother me if the first transformation went away
even for -ffast-math.  It seems egregeously wrong.


r~

Re: Bug in builtin_floor optimization

[Dale Johannesen]

On Aug 23, 2005, at 9:53 AM, Richard Henderson wrote:

> On Tue, Aug 23, 2005 at 09:28:50AM -0600, Roger Sayle wrote:
>> Good catch.  This is indeed a -ffast-math (or more precisely a
>> flag_unsafe_math_optimizations) transformation.  I'd prefer to
>> keep these transformations with -ffast-math, as Jan described them
>> as significantly helping SPEC's mesa when they were added.
>
> Are you sure it was "(float)floor(d)"->"floorf((float)d)" that
> helped mesa and not "(float)floor((double)f)"->"floorf(f)" ?

All the floor calls in mesa seem to be of the form (int)floor((double)f)
or (f - floor((double)f)).  (the casts to double are implicit, 
actually.)

> It wouldn't bother me if the first transformation went away
> even for -ffast-math.  It seems egregeously wrong.

I think I'd prefer this, given that it is not useful in mesa.  Will put
together a patch.