-
Notifications
You must be signed in to change notification settings - Fork 206
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
Modernization of conversions test #1719
Modernization of conversions test #1719
Conversation
Looks good. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found this quite difficult to review, due to large pieces being moved around while functionality is being changed simultaneously. I've left some initial comments, but I'm not sure I'll be able to say I'm confident with the PR. Any attempts to break this into smaller PRs would be appreciated, if possible. :)
This is a consequence of removing huge duplication of code which was requested in an issue discussion. According to my calculation this PR removes 3700 lines of clang formatted code, I think it is worth reviewing.
This wasn't my intention, functionality should stay intact and if you spot such case I will greatly appreciate you pointing it out, thanks. |
I've found another issue with this PR: running a specific test (e.g. |
Corrected, thanks! |
Reviewed an earlier PS however would like to test new commits and need some time because of how long this test takes to complete. |
* Added unification of existing conversions test as preparation for cl_khr_fp16 adaptation * Unified initialization procedures for conversions test. * Completed unification of data structures to handle cl_khr_fp16 * Added support for selective launch of the test * Added half support for test_conversions, work in progres (issue #142, conversions) * Added more work on halfs support for conversions test (issue #142, conversions) * Added cosmetic corrections * Added more cosmetic corrections before opening draft PR * Added corrections related to pre-submit windows build * Added more pre-build related corrections * Added pre-submit ubuntu build related correction * Added more pre-submit related corrections * Divided structures into separate source files (issue #142, conversions) * Added more corrections related to presubmit check * Removed redeclarations due to presubmit check * Added more corrections related to presubmit check arm build * Added cosmetic correction * Adapted modifications from related PR #1719 to avoid merging conflicts * fixed clang format * Added corrections related to code review (cl_khr_fp16 suuport according to issue #142) * Corrections related to macos CI check fail * fix for unclear clang format discrepancy * More corrections related to code review (cl_khr_fp16 for conversions #142) --------- Co-authored-by: Ewan Crawford <[email protected]>
.. in connection to preparations to handle
cl_khr_fp16
extension which was already introduced in original PR. It was separated due to vendor's request.for consistency below are my original remarks for original PR:
In the process of adding cl_khr_fp16 support source code was unified and as result heavily refactored.
Some pieces of code (eg. InitCL fuction) stayed as it was previously for the sake of comparison purposes. For consistency I would suggest to move it to methods (eg. ConversionsTest::SetUp).
Please take a closer look at method DataInfoSpec<>::init under condition is_in_half(). There is new code to initialize array of cl_half values for conversions. It was source of the problem due to some special values which brought undefined results. ATM this code avoids those problematic values.
Capacity of test was decreased. I had some difficult time trying to figure out why this number is so extremely big. I will appreciate your feedback. I limited number of tests to 16 777 216 conversions (uint64_t lastCase = 1000000ULL;)
At my machine intel driver do not implement some of kernel functions which I think should be available: