Enable fp16 in math bruteforce #1433
Conversation
gwawiork
force-pushed
the
fp16_rebase_github_v2
branch
4 times, most recently
from
ea57512
to
28bf69a
Compare
gwawiork
force-pushed
the
fp16_rebase_github_v2
branch
from
28bf69a
to
d25b828
Compare
bashbaug
added
mobica-backlog
| @@ -278,7 +313,7 @@ const Func functionList[] = { | |||
| ENTRY(minmag, 0.0f, 0.0f, FTZ_OFF, binaryF), | |||
| ENTRY(modf, 0.0f, 0.0f, FTZ_OFF, unaryF_two_results), | |||
| ENTRY(nan, 0.0f, 0.0f, FTZ_OFF, unaryF_u), | |||
| ENTRY(nextafter, 0.0f, 0.0f, FTZ_OFF, binaryF), | |||
| ENTRY(nextafter, 0.0f, 0.0f, FTZ_OFF, binaryF_nextafter), | |||
There was a problem hiding this comment.
I think all the ENTRY items here should be updated to use the correct ULPs as defined by the spec - https://registry.khronos.org/OpenCL/specs/3.0-unified/html/OpenCL_Ext.html#cl_khr_fp16-relative-error-as-ulps
This is the same concerns as when I reviewed the previous PR #529 (comment)
There was a problem hiding this comment.
I've corrected ULP values for half test cases. I have two additional remarks which popped up in the process:
- There are couple of tests focused on kernel procedures already covered by
test_relationals, there are also other kernel procedures which should land intest_relationalsfor consistency, list below:
isequal
isfinite
isgreater
isgreaterequa
isinf
isless
islessequal
islessgreater
isnan
isnormal
isnotequal
isordered
isunordered
Should we raise a new issue for that task ?
- It looks like current progress of
test_bruteforce(new PR #1681) does not take into account difference in processing for embedded profile forcl_khr_fp16. Should it be covered by new issue or continuation of development for PR added cl_half support for test_bruteforce #1681 ?
There was a problem hiding this comment.
I would suggest documenting both these points as their own issues to keep the scope of changes in #1681 manageable
There was a problem hiding this comment.
@EwanC can we assume silence means consent ?
There was a problem hiding this comment.
Sure, let's close this. We can always reopen it if closing the PR was an issue for whatever reason.
| @@ -0,0 +1,879 @@ | |||
| // | |||
There was a problem hiding this comment.
Most of the newly added files in this commit are based on older versions of the float/double counterparts. There have been many improvements to the original files since this PR was opened, especially regarding kernel creation, use of cl wrapper objects, and removal of gotos. It would be good to base the new _half.cpp files on a newer version of the _float.cpp/_double.cpp files, which would avoid a lot of (bad) code duplication.
(we might wish to wait for #1634 to land before working on this).
There was a problem hiding this comment.
@svenvh sounds good. It seems like there is no other option than align to mentioned modernization - after pulling and solving conflicts this branch doesn't build successfully anymore. I am working on adaptation right now so if you have any suggestions I will appreciate it.
There was a problem hiding this comment.
I would probably start afresh from the latest main branch.
There are many things you should still be able to reuse from this PR, such as the plumbing in main.cpp, tables with special values and the reference result computations.
Kernel code generation should be much easier now: it should only be a matter of adding a half ParameterType to common.h and common.cpp.
neiltrevett
added
mobica-backlog
|
Closing this PR as it's superseded by #1681 and not been worked on in over a year. If this is a problem, feel free to reopen the PR. |
This is almost the duplicate of #529 after rebase.