Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added cl_khr_fp16 extension support for test_explicit_s2v from basic #1713

Merged
merged 13 commits into from
Oct 17, 2023

Conversation

shajder
Copy link
Contributor

@shajder shajder commented Apr 27, 2023

According to issue description (basic section):

#142

Additional remark (1): original test skips conversions between different types, in other words supported conversions were eg. char scalar->char vector, int scalar->int vector, float scalar->float vector and so on. This PR extend the scope of original test to integral scalar types->integral vector types, floating scalar types->floating vector types

Additional remark (2): test fails with my Intel device and kernel with half conversions:
ERROR: Explicit cast of scalar float to vector half2 FAILED; skipping other half vector tests
I will appreciate additional attention when it comes to testing cl_khr_fp16 with this test

@shajder shajder marked this pull request as ready for review June 12, 2023 08:15
@EwanC
Copy link
Contributor

EwanC commented Jun 14, 2023

Regarding

Additional remark (2): test fails with my Intel device and kernel with half conversions:
ERROR: Explicit cast of scalar float to vector half2 FAILED; skipping other half vector tests
I will appreciate additional attention when it comes to testing cl_khr_fp16 with this test

I'm also seeing this error locally:

ERROR: Output value 0:0 does not validate for size 2:2!
       Input:   0x4dbe2551
       Actual:  0x7c007c00 0x7c007c00 0x7c007c00 0x7c007c00
ERROR: Explicit cast of scalar float to vector half2 FAILED; skipping other half vector tests

0x4dbe2551 is 398764576.0f which is outside the range of half, which is why the half2 is (0x7c00, 0x7c00), i.e INF values. So think we might need to check for overflow during the verification.

@shajder
Copy link
Contributor Author

shajder commented Jun 14, 2023

@EwanC I just added missing part of convert_explicit_value to support kHalf ExplicitType. Test passes now but I will appreciate reviewing it more carefully - I am not quite sure it is 100% correct

EwanC
EwanC previously approved these changes Jun 14, 2023
Copy link
Contributor

@EwanC EwanC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passes for me now 🙂 HALF_ROUND_CASE has a lot of code duplication with FLOAT_ROUND_CASE & DOUBLE_ROUND_CASE which would be nice to improve upon but i'm okay with the change without it.

@lakshmih
Copy link
Contributor

Reviewing

Copy link
Contributor

@lakshmih lakshmih left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is failing (on below kernel) with an error that cl_khr_fp16 is not enabled. Can we add the pragma for kernels that use half?

__kernel void test_conversion(__global float *sourceValues, __global half2 *destValues )
{
int tid = get_global_id(0);
float src = sourceValues[tid];

     destValues[tid] = (half2)src;

 }


#include "procs.h"
#include "harness/conversions.h"
#include "harness/typeWrappers.h"

// clang-format off

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs "#pragma OPENCL EXTENSION cl_khr_fp16 : enable\n" here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Corrected - good find, thanks!

@bcalidas
Copy link

We are seeing failures with the latest commit. Debugging at our end. Need some more time.
Thanks,
Balaji

lakshmih
lakshmih previously approved these changes Sep 11, 2023
@lakshmih
Copy link
Contributor

Added a patch that does the NAN check.

Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems OK but I have a few questions, mostly around NaN handling.

The additional test coverage does increase the runtime of this sub-test, but it still runs in ~30 seconds on my system so probably not worth worrying about (yet).

test_conformance/basic/test_explicit_s2v.cpp Outdated Show resolved Hide resolved
test_conformance/basic/test_explicit_s2v.cpp Outdated Show resolved Hide resolved
@bashbaug
Copy link
Contributor

bashbaug commented Oct 3, 2023

Discussed in the October 3rd teleconference. Will wait a week to address the comments above.

Use std::isnan for float/double types.

Change-Id: I005bddccaa3f8490ac59b2aa431ed315733ad143
bashbaug
bashbaug previously approved these changes Oct 10, 2023
test_conformance/basic/test_explicit_s2v.cpp Outdated Show resolved Hide resolved
Change-Id: I671ed826a9631fbbc66d0aa9b674ab00124c7967
bashbaug
bashbaug previously approved these changes Oct 12, 2023
Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with one small nice-to-have - thanks!

test_conformance/basic/test_explicit_s2v.cpp Outdated Show resolved Hide resolved
Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging as discussed in the October 17th teleconference.

@bashbaug bashbaug merged commit 72bb711 into KhronosGroup:main Oct 17, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants