[PATCH 1/7] relational: create re-usable macros for relational declarations

relational.h includes relational macros for defining functions which need to
return 1 for scalar true and -1 for vector true.

I believe that this is the only place that this behavior is required, so the
macro is placed at its lowest useful level (same directory as it is used in).

This also creates re-usable unary/binary declaration and floatn includes which
should simplify relational builtin declarations.

Mostly patterned off of include/math/[binary_decl|unary_decl|floatn].inc
but with required changes for relational functions.

Signed-off-by: Aaron Watry <awatry@gmail.com>

Vector true is -1, not 1, which means we need to use the relational unary
macro instead of the normal unary builtin one.

Signed-off-by: Aaron Watry <awatry@gmail.com>

Signed-off-by: Aaron Watry <awatry@gmail.com>

v2: Use relational macros instead of hand-rolled macros

Signed-off-by: Aaron Watry <awatry@gmail.com>

v2: Use relational macros instead of hand-rolled macros

Signed-off-by: Aaron Watry <awatry@gmail.com>

v2: Use relational macros instead of hand-rolled ones

Signed-off-by: Aaron Watry <awatry@gmail.com>

Signed-off-by: Aaron Watry <awatry@gmail.com>

Should these go under an #if __OPENCL_VERSION__ >= CL_VERSION_1_1? Were these really not in 1.0?

Hi Aaron,

I think you should follow the hexadecimal floating-point format used for the other defines.

Jeroen

Huh? These aren't under a version restriction. These defines were
available in CL 1.0, we just never declared them before (page 230 of
cl 1.0 spec)

Sure. I'll respin this using hex fp format...

--Aaron

Huh? These aren't under a version restriction. These defines were
available in CL 1.0, we just never declared them before (page 230 of
cl 1.0 spec)

OK, your message says from CL 1.1 so it sounded like they were new additions in 1.1

Signed-off-by: Aaron Watry <awatry@gmail.com>
---
generic/include/clc/relational/signbit.h | 21 +++-------
generic/lib/relational/signbit.cl | 72 +-------------------------------
2 files changed, 8 insertions(+), 85 deletions(-)

diff --git a/generic/include/clc/relational/signbit.h b/generic/include/clc/relational/signbit.h
index 774d6e0..41e5284 100644
--- a/generic/include/clc/relational/signbit.h
+++ b/generic/include/clc/relational/signbit.h
@@ -1,18 +1,9 @@
+#undef signbit

What happens if someone compiles a kernel with:
-Dsignbit=__builtin_amdgpu_signbit

Won't the undef here break this kernel? Are these kinds of defines
even allowed?

-Tom

Signed-off-by: Aaron Watry <awatry@gmail.com>

generic/include/clc/relational/signbit.h | 21 ++±------
generic/lib/relational/signbit.cl | 72 ±------------------------------
2 files changed, 8 insertions(+), 85 deletions(-)

diff --git a/generic/include/clc/relational/signbit.h b/generic/include/clc/relational/signbit.h
index 774d6e0…41e5284 100644
— a/generic/include/clc/relational/signbit.h
+++ b/generic/include/clc/relational/signbit.h
@@ -1,18 +1,9 @@
+#undef signbit

What happens if someone compiles a kernel with:
-Dsignbit=__builtin_amdgpu_signbit

Won’t the undef here break this kernel? Are these kinds of defines
even allowed?

-Tom

I also think defining most library function as macros could have unpleasant consequences on warnings. Is the final clc header preprocessed at install time or anything like that? I also noticed that none of the function are currently marked with attribute((const)), but nearly every function should have it.

Yes, it probably will. I was just following the established pattern
in almost all of the math functions (at least ceil, cos, exp10, exp2,
exp, fabs, floor, fma, fmax, fmin, log2, log, pow, rint, round, sin,
and trunc, maybe others too).

Maybe we should consider fixing those as well.

--Aaron

Yes, it probably will. I was just following the established pattern
in almost all of the math functions (at least ceil, cos, exp10, exp2,
exp, fabs, floor, fma, fmax, fmin, log2, log, pow, rint, round, sin,
and trunc, maybe others too).

Ok, so patches 1-6 LGTM.

Maybe we should consider fixing those as well.

Someone can do this as a follow on series.

-Tom

>> Signed-off-by: Aaron Watry <awatry@gmail.com>
>> ---
>> generic/include/clc/relational/signbit.h | 21 +++-------
>> generic/lib/relational/signbit.cl | 72 +-------------------------------
>> 2 files changed, 8 insertions(+), 85 deletions(-)
>>
>> diff --git a/generic/include/clc/relational/signbit.h b/generic/include/clc/relational/signbit.h
>> index 774d6e0..41e5284 100644
>> --- a/generic/include/clc/relational/signbit.h
>> +++ b/generic/include/clc/relational/signbit.h
>> @@ -1,18 +1,9 @@
>> +#undef signbit
>>
>
> What happens if someone compiles a kernel with:
> -Dsignbit=__builtin_amdgpu_signbit
>
> Won't the undef here break this kernel? Are these kinds of defines
> even allowed?
>
> -Tom

I also think defining most library function as macros could have unpleasant consequences on warnings. Is the final clc header preprocessed at install time or anything like that? I also noticed that none of the function are currently marked with __attribute__((const)), but nearly every function should have it.

All of the headers are installed. Can we fix all of our macro issues by just
installing a single preprocessed clc.h. Are there any downsides to doing this?

What does __attribute__((const)) do?

-Tom

Signed-off-by: Aaron Watry <awatry@gmail.com>

generic/include/clc/relational/signbit.h | 21 ++±------
generic/lib/relational/signbit.cl | 72 ±------------------------------
2 files changed, 8 insertions(+), 85 deletions(-)

diff --git a/generic/include/clc/relational/signbit.h b/generic/include/clc/relational/signbit.h
index 774d6e0…41e5284 100644
— a/generic/include/clc/relational/signbit.h
+++ b/generic/include/clc/relational/signbit.h
@@ -1,18 +1,9 @@
+#undef signbit

What happens if someone compiles a kernel with:
-Dsignbit=__builtin_amdgpu_signbit

Won’t the undef here break this kernel? Are these kinds of defines
even allowed?

-Tom

I also think defining most library function as macros could have unpleasant consequences on warnings. Is the final clc header preprocessed at install time or anything like that? I also noticed that none of the function are currently marked with attribute((const)), but nearly every function should have it.

All of the headers are installed. Can we fix all of our macro issues by just
installing a single preprocessed clc.h. Are there any downsides to doing this?

I’m not sure. The installed header should really go one step further and be a precompiled header to help with compile time.

What does attribute((const)) do?

It’s the C equivalent of readnone (and pure is readonly). It’s probably inferable in most cases assuming the intrinsics are correct, but it might help with compile time to have it there from the beginning.

Signed-off-by: Aaron Watry <awatry@gmail.com>
---
generic/include/clc/relational/signbit.h | 21 +++-------
generic/lib/relational/signbit.cl | 72
+-------------------------------
2 files changed, 8 insertions(+), 85 deletions(-)

diff --git a/generic/include/clc/relational/signbit.h
b/generic/include/clc/relational/signbit.h
index 774d6e0..41e5284 100644
--- a/generic/include/clc/relational/signbit.h
+++ b/generic/include/clc/relational/signbit.h
@@ -1,18 +1,9 @@
+#undef signbit

What happens if someone compiles a kernel with:
-Dsignbit=__builtin_amdgpu_signbit

Won't the undef here break this kernel? Are these kinds of defines
even allowed?

-Tom

I also think defining most library function as macros could have unpleasant
consequences on warnings. Is the final clc header preprocessed at install
time or anything like that? I also noticed that none of the function are
currently marked with __attribute__((const)), but nearly every function
should have it.

All of the headers are installed. Can we fix all of our macro issues by
just
installing a single preprocessed clc.h. Are there any downsides to doing
this?

If we do this, do we have to do anything special for cases where
half/double aren't supported? We probably won't know that until
runtime, and it'd be nice to know at compile time that you're
attempting to use something that won't work rather than link/execution
time. Not sure, just asking.

--Aaron

>
>
>
>
>
>
> Signed-off-by: Aaron Watry <awatry@gmail.com>
> ---
> generic/include/clc/relational/signbit.h | 21 +++-------
> generic/lib/relational/signbit.cl | 72
> +-------------------------------
> 2 files changed, 8 insertions(+), 85 deletions(-)
>
> diff --git a/generic/include/clc/relational/signbit.h
> b/generic/include/clc/relational/signbit.h
> index 774d6e0..41e5284 100644
> --- a/generic/include/clc/relational/signbit.h
> +++ b/generic/include/clc/relational/signbit.h
> @@ -1,18 +1,9 @@
> +#undef signbit
>
>
> What happens if someone compiles a kernel with:
> -Dsignbit=__builtin_amdgpu_signbit
>
> Won't the undef here break this kernel? Are these kinds of defines
> even allowed?
>
> -Tom
>
>
> I also think defining most library function as macros could have unpleasant
> consequences on warnings. Is the final clc header preprocessed at install
> time or anything like that? I also noticed that none of the function are
> currently marked with __attribute__((const)), but nearly every function
> should have it.
>
>
> All of the headers are installed. Can we fix all of our macro issues by
> just
> installing a single preprocessed clc.h. Are there any downsides to doing
> this?

If we do this, do we have to do anything special for cases where
half/double aren't supported? We probably won't know that until
runtime, and it'd be nice to know at compile time that you're
attempting to use something that won't work rather than link/execution
time. Not sure, just asking.

This is a problem now since we unconditionally add -Dcl_khr_fp64 to the
list of defines in libclc.

I was planning to add cl_khr_fp64 to the list of target defines in clang, so
that doubles (and also half types) would be enabled automatically on targets
that support it.