[PATCH 1/4] Implement atan builtin

The double version still uses @llvm.cos.

This double version still uses @llvm.sin.

This double version still uses @llvm.sin.

native_sin should still use @llvm.sin. I think it’s currently defined to just use regular sin function though

Hi Tom,

atan and atan2 clearly need implementations, as there are no intrinsics for them. However, why are implementations of cos and sin for float needed? The intrinsics are typed so they could just be expanded appropriately in the backend.

On another note: It’s difficult to check the code in the patches. Is the intention to implement some standard textbook algorithm?

Jeroen

Hi Tom,

atan and atan2 clearly need implementations, as there are no intrinsics for them. However, why are implementations of cos and sin for float needed? The intrinsics are typed so they could just be expanded appropriately in the backend.

On another note: It’s difficult to check the code in the patches. Is the intention to implement some standard textbook algorithm?

Jeroen

The intrinsics do not have the precision or well defined edge case behavior required by the standard.The LLVM math intrinsics are sufficient for native_* math functions, and maybe in some of the fast math modes, but not as the standard function behavior.

Hi Tom,

atan and atan2 clearly need implementations, as there are no intrinsics for them. However, why are implementations of cos and sin for float needed? The intrinsics are typed so they could just be expanded appropriately in the backend.

I think it's good to provide a generic version otherwise targets will
end up duplicating code in the backends. If a target wants to use the
intrinsic or some other implementation, they can still override the
default in libclc.

On another note: It’s difficult to check the code in the patches. Is the intention to implement some standard textbook algorithm?

I added tests cases for these to piglit:
http://lists.freedesktop.org/archives/piglit/2014-July/011584.html
If you know of any edge cases I will add them.

To be honest I don't know anything about the algorithms being used.
These functions were ported from the builtin library used by AMD's
proprietary OpenCL implementation, and as far as I know, they are
correct.

-Tom

I'm going to commit these tomorrow if there are no objections.

-Tom

No objection here. I am assuming that the formulas are correct based
on the previous comment about them coming straight from another AMD
math library.

Only question I have is if the defined macros in math.h will interfere
with kernels that use the same names for their own macros. The header
is in generic/lib/math, so it's not going to be installed in the
include directory, but I just wanted to make sure that those macros
were getting resolved before user kernels attempted to redefine them
(e.g. macros substituted when creating the .bc files).

--Aaron

No objection here. I am assuming that the formulas are correct based
on the previous comment about them coming straight from another AMD
math library.

Only question I have is if the defined macros in math.h will interfere
with kernels that use the same names for their own macros. The header
is in generic/lib/math, so it's not going to be installed in the
include directory, but I just wanted to make sure that those macros
were getting resolved before user kernels attempted to redefine them
(e.g. macros substituted when creating the .bc files).

This shouldn't be a problem as long as it stays in the lib/ subdirectory.
Since clover links against the .bc files all the macros have already been resolved.

-Tom

No objection here. I am assuming that the formulas are correct based
on the previous comment about them coming straight from another AMD
math library.

Only question I have is if the defined macros in math.h will interfere
with kernels that use the same names for their own macros. The header
is in generic/lib/math, so it's not going to be installed in the
include directory, but I just wanted to make sure that those macros
were getting resolved before user kernels attempted to redefine them
(e.g. macros substituted when creating the .bc files).

This shouldn't be a problem as long as it stays in the lib/ subdirectory.
Since clover links against the .bc files all the macros have already been resolved.

That's what I thought, just wanted to make sure.

Hi Tom,

I was working on implementing asin/acos and a few others recently, and
I noticed that atan2 doesn't actually run on evergreen (CEDAR).

Using the attached piglit test, I get the following error:
LLVM triggered Diagnostic Handler: unsupported call to function
__extendsfdf2 in test_1_atan2_float

Note that the atan function passes all of the tests, it's just the
atan2 function which doesn't seem to work on evergreen (works on
radeonsi, however).

--Aaron

builtin-float-atan2-1.0.generated.cl (8.99 KB)

Hi Tom,

I was working on implementing asin/acos and a few others recently, and
I noticed that atan2 doesn't actually run on evergreen (CEDAR).

Using the attached piglit test, I get the following error:
LLVM triggered Diagnostic Handler: unsupported call to function
__extendsfdf2 in test_1_atan2_float

__extendsfdf2 is the library function for converting f32 to f64, so this error
means evergreen does not implement this conversion. I think this is a bug
in the way we report double support, since evergreen should not be using any
double types. I will look into this.

-Tom