Re: [COMMITTERS] pgsql: Add trigonometric functions that work in degrees.

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> On 04/25/2016 03:09 PM, Tom Lane wrote:
>> I'm going to go ahead and push this, because it seems clearly more robust
>> than what we have. But I'd appreciate a report on whether it fixes your
>> issue.

> 6b1a213bbd6599228b2b67f7552ff7cc378797bf did not fix it.

OK ... but it's still a good change, because it removes the assumption
that the compiler won't inline init_degree_constants().

> Attached is the assembler output (-O0) from float.c as of that commit.

Ah. Here's the problem: line 1873 is

return (asin(x) / asin_0_5) * 30.0;

and that compiles into

subl $8, %esp
pushl -12(%ebp) ... push x
pushl -16(%ebp)
call asin ... call asin()
addl $16, %esp
fldl asin_0_5 ... divide by asin_0_5
fdivrp %st, %st(1)
fldt .LC46 ... multiply by 30.0
fmulp %st, %st(1)
fstpl -24(%ebp) ... round to 64 bits
fldl -24(%ebp)

Evidently, asin() is returning an 80-bit result, and that's not
getting rounded to double width before we divide by asin_0_5.

The least ugly change that might fix this is to explicitly cast the
result of asin to double:

return ((float8) asin(x) / asin_0_5) * 30.0;

However, I'm not certain that that would do anything; the compiler
might feel that the result of asin() is already double. The next
messier answer is to explicitly store the function result in a local
variable:

{
float8 asin_x = asin(x);
return (asin_x / asin_0_5) * 30.0;
}

A sufficiently cavalier compiler might choose to optimize that away,
too. A look at gcc's manual suggests that we might need to use
the -ffloat-store option to guarantee it will work; which is ugly
and I'd prefer not to turn that on globally anyway. If it comes to
that, probably the better answer is to turn asin_x into a global
variable, similarly to what we just did with the constants.

Can you try the above variants of line 1873 and see if either of them
fixes the issue for you?

regards, tom lane