added cl_half support for test_bruteforce #1681

shajder · 2023-03-23T17:07:31Z

According to issue description (bruteforce section):

Additional remark: This PR is based on @gwawiork work from PR #1433. Due to long approval process it was necessary to rebase and adapt to #1634 modernization. Due to reassignment to mobica-backlog it was more convenient to introduce new PR (which was discussed with Grzegorz).

…ronosGroup#142, bruteforce)

test_conformance/math_brute_force/CMakeLists.txt

lakshmih · 2023-04-10T17:39:17Z

Testing / reviewing.

lakshmih · 2023-04-13T18:55:50Z

We would like to double check results at our end, can we get some more time.

…sts for cl_half (issue KhronosGroup#142, bruteforce)

test_conformance/math_brute_force/CMakeLists.txt

test_conformance/math_brute_force/macro_unary_half.cpp

test_conformance/math_brute_force/reference_math.cpp

…test

test_conformance/math_brute_force/ternary_half.cpp

lakshmih

Requesting changes.

test_conformance/math_brute_force/unary_two_results_half.cpp

…hronosGroup#142)

svenvh

This looks good to me now; thanks!

bashbaug

I did a quick run with these changes on our implementation with:

test_bruteforce -f -r -w -1

Aside from the known failure with fract I'm also seeing a failure with divide_cr:

 83:       divide_cr fp16     
ERROR: divide_cr: -0.500000 ulp error at {-0x1.394p-12 (0x8ce5), -0x1.cp+5 (0xd300)}
Expected: 0x1.68p-18  (half 0x005a) 
Actual: 0x1.64p-18 (half 0x0059) at index: 22220
ERROR: ThreadPool_Do: TestHalf failed
! (CL_DEVICE_NOT_FOUND from /home/bashbaug/git/OpenCL-CTS/test_conformance/math_brute_force/binary_operator_half.cpp:667)
divide_cr FAILED

I haven't investigated further but I wanted to raise this ASAP.

bashbaug · 2023-12-05T16:06:54Z

fma is failing also, if I take off -w:

 25:             fma fp16     ....
ERROR: fma: -0.500000 ulp error at {0x1.eacp+7, 0x1.3f4p+4, 0x1.c04p+14} ({0x5bab, 0x4cfd, 0x7701}): *0x1.068p+15 vs. 0x1.064p+15
fma FAILED

bashbaug · 2023-12-05T16:32:36Z

I'm also seeing a failure with divide_cr:

It looks like the tests are checking for a correctly rounded result in this case, but it's not clear this is required. The build option specifically says (emphasis mine):

"-cl-fp32-correctly-rounded-divide-sqrt
The -cl-fp32-correctly-rounded-divide-sqrt build option to clBuildProgram or clCompileProgram allows an application to specify that single precision floating-point divide (x/y and 1/x) and sqrt used in the program source are correctly rounded. If this build option is not specified, the minimum numerical accuracy of single precision floating-point divide and sqrt are as defined in the OpenCL C or OpenCL SPIR-V Environment specifications."

I think the safest thing to do would be to disable the divide_cr test for fp16 while we figure out what to do with it.

I'll look at fma next, but probably not before the teleconference.

svenvh · 2023-12-05T17:06:33Z

It looks like the tests are checking for a correctly rounded result in this case, but it's not clear this is required. The build option specifically says (emphasis mine):

"-cl-fp32-correctly-rounded-divide-sqrt The -cl-fp32-correctly-rounded-divide-sqrt build option to clBuildProgram or clCompileProgram allows an application to specify that single precision floating-point divide (x/y and 1/x) and sqrt used in the program source are correctly rounded. If this build option is not specified, the minimum numerical accuracy of single precision floating-point divide and sqrt are as defined in the OpenCL C or OpenCL SPIR-V Environment specifications."

I think the safest thing to do would be to disable the divide_cr test for fp16 while we figure out what to do with it.

Correctly rounded results for fp16-divide and fp16-sqrt are not dictated by CL_FP_CORRECTLY_ROUNDED_DIVIDE_SQRT indeed. Instead, "Table 11. ULP Values for Half Precision Floating-Point Arithmetic Operations" of the extension spec states that fp16-divide and fp16-sqrt need to be correctly rounded for Full Profile, and <= 1 ULP for Embedded Profile.

That made me realize that the additions to function_list.cpp only add the Full Profile ULP values. For quite a few functions (e.g. cos, exp, ...), the Embedded Profile ULP value is different, but this is not taken into account in this patch.

lakshmih · 2023-12-12T15:49:03Z

We are seeing failures in multiple tests with this patch and haven't fully resolved them to bugs in the test or in our implementation.

List:

Add -> Fails when one or more inputs are subnormal.
Fma
Fract
Multiply
Remquo
Subtract
Tanpi

lakshmih · 2023-12-12T16:39:13Z

test_conformance/math_brute_force/binary_operator_half.cpp

+                        float err2, err3;
+
+                        correct2 = HTF(func.f_ff(0.0, s2[j]));
+                        correct3 = HTF(func.f_ff(-0.0, s2[j]));


The half_to_float is unnecessary here since func.f_ff() returns a double. Similar changes are needed elsewhere in the code as well.

correct2 = func.f_ff(0.0, s2[j]);
correct3 = func.f_ff(-0.0, s2[j]);

bashbaug · 2023-12-12T16:43:46Z

We're still debugging a few unexpected failures on our side, also.

Can we merge these changes but with half testing disabled by default, able to be enabled via command line arguments? Then, when debug is complete and any issues are fixed, we can re-enable half testing by default?

svenvh · 2023-12-12T17:10:32Z

Can we merge these changes but with half testing disabled by default, able to be enabled via command line arguments? Then, when debug is complete and any issues are fixed, we can re-enable half testing by default?

That sounds good to me; as this is a very large patch, it will be hard to get everything right the first time without the patch going stale.

bashbaug · 2023-12-16T00:29:36Z

I just created an fp16-staging branch, as we discussed. If this PR is retargeted at this branch I will merge it, also as we discussed.

shajder · 2023-12-18T17:13:14Z

I just created an fp16-staging branch, as we discussed. If this PR is retargeted at this branch I will merge it, also as we discussed.

@bashbaug I've created new PR to be merged with fp16-staging branch

Nuullll · 2023-12-20T06:23:11Z

Got 2 fails with test_bruteforce -f -r -w -1 on our device. Investigating:

fract...
 57:           fract fp64     ................Wimp pass	{    0.00,     0.00} @ {0x0p+0, 0x0p+0}
 58:           fract fp16     
ERROR: fract: {0.125000, 0.000000} ulp error at -0x1p-14: *{0x1.ffcp-1, -0x1p+0} vs. {0x1p+0, -0x1p+0}
fract FAILED

nextafter...
117:       nextafter fp64     ................Wimp pass	    0.00 @ {0x0p+0, 0x0p+0}
118:       nextafter fp16     
ERROR: nextafter: -nan ulp error at {0x0p+0 (0x0000), 0x1.3ccp-2 (0x34f3)}
Expected: 0x1p-24  (half 0x0001) 
Actual: -nan (half 0xffff) at index: 54

ERROR: nextafter: -nan ulp error at {0x0p+0 (0x0000), 0x1.578p-6 (0x255e)}
Expected: 0x1p-24  (half 0x0001) 
Actual: -nan (half 0xffff) at index: 4670

hvdijk · 2024-01-17T18:20:27Z

I'm seeing

ERROR: fma: -0.500000 ulp error at {0x1.eacp+7, 0x1.3f4p+4, 0x1.c04p+14} ({0x5bab, 0x4cfd, 0x7701}): *0x1.068p+15 vs. 0x1.064p+15
fma FAILED

on our implementation. 0x1.eacp+7 * 0x1.3f4p+4 + 0x1.c04p+14 is exactly 0x1.065fffp+15. Rounding to half-precision directly should give 0x1.064p+15. What I suspect one version is doing is rounding to single-precision first to give 0x1.066p+15, and then rounding that to half-precision to give 0x1.068p+15, which is the wrong result. But it never actually says which of the two values is the value the CTS considers correct, which would tell us whether it is the CTS that is doing the wrong calculation, or our implementation. Is the * meant to indicate the correct version? "expected: <...>, actual: <...>" would be clearer, in my opinion. Edit: Have confirmed that the CTS is the one giving the incorrect result.

bashbaug · 2024-01-23T17:24:35Z

Removing "focused review", since we are reviewing and merging changes into the staging branch now.

hvdijk · 2024-01-24T10:00:00Z

I'm seeing
ERROR: fma: -0.500000 ulp error at {0x1.eacp+7, 0x1.3f4p+4, 0x1.c04p+14} ({0x5bab, 0x4cfd, 0x7701}): *0x1.068p+15 vs. 0x1.064p+15
fma FAILED
on our implementation. 0x1.eacp+7 * 0x1.3f4p+4 + 0x1.c04p+14 is exactly 0x1.065fffp+15. Rounding to half-precision directly should give 0x1.064p+15. What I suspect one version is doing is rounding to single-precision first to give 0x1.066p+15, and then rounding that to half-precision to give 0x1.068p+15, which is the wrong result. But it never actually says which of the two values is the value the CTS considers correct, which would tell us whether it is the CTS that is doing the wrong calculation, or our implementation. Is the * meant to indicate the correct version? "expected: <...>, actual: <...>" would be clearer, in my opinion. Edit: Have confirmed that the CTS is the one giving the incorrect result.

I was asked to open a PR for this against fp16-staging for this; done as #1882.

Nuullll · 2024-02-04T07:07:56Z

Got 2 fails with test_bruteforce -f -r -w -1 on our device. Investigating:

fract...
 57:           fract fp64     ................Wimp pass	{    0.00,     0.00} @ {0x0p+0, 0x0p+0}
 58:           fract fp16     
ERROR: fract: {0.125000, 0.000000} ulp error at -0x1p-14: *{0x1.ffcp-1, -0x1p+0} vs. {0x1p+0, -0x1p+0}
fract FAILED

nextafter...
117:       nextafter fp64     ................Wimp pass	    0.00 @ {0x0p+0, 0x0p+0}
118:       nextafter fp16     
ERROR: nextafter: -nan ulp error at {0x0p+0 (0x0000), 0x1.3ccp-2 (0x34f3)}
Expected: 0x1p-24  (half 0x0001) 
Actual: -nan (half 0xffff) at index: 54

ERROR: nextafter: -nan ulp error at {0x0p+0 (0x0000), 0x1.578p-6 (0x255e)}
Expected: 0x1p-24  (half 0x0001) 
Actual: -nan (half 0xffff) at index: 4670

The fract and nextafter errors were caused by our implementation bugs. After fixing our bugs, test_bruteforce -f -r -w is passing on our device.

cycheng · 2024-05-16T04:36:13Z

When is this PR expected to be merged?

shajder · 2024-06-19T06:40:12Z

This PR was merged to main branch along with this

gwawiork and others added 6 commits June 8, 2022 10:27

Enable fp16 in math bruteforce

d25b828

Merge branch 'main' into fp16_bruteforce

b2ae560

Added modernization of remaining half tests for consistency (issue Kh…

30467ce

…ronosGroup#142, bruteforce)

Added kernel types related corrections

5f3f2ec

Added more fixes and general cleanup

cf97168

Corrected ULP values for half tests (issue KhronosGroup#142, bruteforce)

cc8db3b

shajder mentioned this pull request Mar 23, 2023

Enable fp16 in math bruteforce #1433

Closed

Corrected presubmit check for clang format

dd42b07

shajder marked this pull request as ready for review March 24, 2023 08:04

svenvh self-requested a review March 27, 2023 09:16

EwanC added the focused review label Mar 28, 2023

shajder mentioned this pull request Mar 29, 2023

Support for cl_khr_fp16 with embedded profile in test_bruteforce #1685

Open

EwanC reviewed Apr 3, 2023

View reviewed changes

test_conformance/math_brute_force/CMakeLists.txt Show resolved Hide resolved

Added support for ternary, unary_two_result and unary_two_result_i te…

04033bf

…sts for cl_half (issue KhronosGroup#142, bruteforce)

lakshmih reviewed Apr 20, 2023

View reviewed changes

test_conformance/math_brute_force/CMakeLists.txt Show resolved Hide resolved

lakshmih reviewed May 8, 2023

View reviewed changes

test_conformance/math_brute_force/macro_unary_half.cpp Outdated Show resolved Hide resolved

shajder added 3 commits May 9, 2023 12:50

Added missing condition due to vendor's review

70cef0c

Merge branch 'main' into fp16_bruteforce

6f3ccf5

code format correction

4e9938e

lakshmih reviewed May 31, 2023

View reviewed changes

test_conformance/math_brute_force/reference_math.cpp Show resolved Hide resolved

shajder added 4 commits June 2, 2023 13:32

Added check for lack of support for denormals in binary_half scenario

b9ae99b

Corrected procedure to compute nextafter cl_half for flush-to-zero mode

5b313bd

Added correction for external check of reference value for nextafter …

0c937a9

…test

Merge branch 'main' into fp16_bruteforce

0625ef6

lakshmih reviewed Jun 12, 2023

View reviewed changes

test_conformance/math_brute_force/ternary_half.cpp Outdated Show resolved Hide resolved

lakshmih reviewed Jun 12, 2023

View reviewed changes

test_conformance/math_brute_force/ternary_half.cpp Outdated Show resolved Hide resolved

lakshmih requested changes Jun 12, 2023

View reviewed changes

Added correction due to code review request

867df9f

svenvh reviewed Nov 17, 2023

View reviewed changes

test_conformance/math_brute_force/unary_two_results_half.cpp Show resolved Hide resolved

more consistency corrections for cl_fp16_khr supported tests

c49e825

bashbaug mentioned this pull request Nov 27, 2023

incorrect result for fp16 fract function for small negative inputs intel/intel-graphics-compiler#307

Closed

shajder added 3 commits November 28, 2023 11:52

Corrections related to code review (bureforce KhronosGroup#142)

6f85906

Correction for i_unary_half test capacity

64db7f5

Corrections related to capacity of cl_khr_fp16 tests in bruteforce (K…

2944911

…hronosGroup#142)

svenvh approved these changes Nov 29, 2023

View reviewed changes

bashbaug reviewed Dec 5, 2023

View reviewed changes

lakshmih reviewed Dec 12, 2023

View reviewed changes

shajder mentioned this pull request Dec 18, 2023

Fp16 math bruteforce staging #1863

Merged

bashbaug removed the focused review label Jan 23, 2024

svenvh mentioned this pull request Mar 18, 2024

determine and fix the conditions when the math brute force "divide_cr" test should be run #1901

Closed

shajder closed this Jun 19, 2024

shajder reopened this Jun 19, 2024

shajder closed this Jun 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added cl_half support for test_bruteforce #1681

added cl_half support for test_bruteforce #1681

shajder commented Mar 23, 2023

lakshmih commented Apr 10, 2023

lakshmih commented Apr 13, 2023

lakshmih left a comment

svenvh left a comment

bashbaug left a comment

bashbaug commented Dec 5, 2023

bashbaug commented Dec 5, 2023

svenvh commented Dec 5, 2023

lakshmih commented Dec 12, 2023 •

edited

Loading

lakshmih Dec 12, 2023

bashbaug commented Dec 12, 2023

svenvh commented Dec 12, 2023

bashbaug commented Dec 16, 2023

shajder commented Dec 18, 2023

Nuullll commented Dec 20, 2023

hvdijk commented Jan 17, 2024 •

edited

Loading

bashbaug commented Jan 23, 2024

hvdijk commented Jan 24, 2024

Nuullll commented Feb 4, 2024

cycheng commented May 16, 2024

shajder commented Jun 19, 2024

added cl_half support for test_bruteforce #1681

added cl_half support for test_bruteforce #1681

Conversation

shajder commented Mar 23, 2023

lakshmih commented Apr 10, 2023

lakshmih commented Apr 13, 2023

lakshmih left a comment

Choose a reason for hiding this comment

svenvh left a comment

Choose a reason for hiding this comment

bashbaug left a comment

Choose a reason for hiding this comment

bashbaug commented Dec 5, 2023

bashbaug commented Dec 5, 2023

svenvh commented Dec 5, 2023

lakshmih commented Dec 12, 2023 • edited Loading

lakshmih Dec 12, 2023

Choose a reason for hiding this comment

bashbaug commented Dec 12, 2023

svenvh commented Dec 12, 2023

bashbaug commented Dec 16, 2023

shajder commented Dec 18, 2023

Nuullll commented Dec 20, 2023

hvdijk commented Jan 17, 2024 • edited Loading

bashbaug commented Jan 23, 2024

hvdijk commented Jan 24, 2024

Nuullll commented Feb 4, 2024

cycheng commented May 16, 2024

shajder commented Jun 19, 2024

lakshmih commented Dec 12, 2023 •

edited

Loading

hvdijk commented Jan 17, 2024 •

edited

Loading