Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Disable cl_khr_fp16 for the Environments unsupporting cl_khr_fp16 #266
Disable cl_khr_fp16 for the Environments unsupporting cl_khr_fp16 #266
Changes from 7 commits
396cc35
d024318
324126b
93b0e71
aa89256
909ec19
d5e4865
aaf590c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why didn't you just remove cl_khr_fp16 for all environments but disable it only if it doesn't support it?
Do you have any definite disadvantages or problems when you treat half as int16 on a cl_khr_fp16 machine? If yes, please leave comments on the code, otherwise (yet) please make the code simple.
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 think that it's better in performance to use
half
implemented as hardware. If we will support allhalf
operations, the performance given from nativehalf
in supported environments is more important.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.
We have not proven the ability neither decided to do that, must continue to discuss on #265 .
Then, until to decide it, the half performance is not important even if it has native half support.
Adding
cl_khr_fp16
is now too superfluous to just disable it #264 .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.
Sure.
So, you want to just remove
cl_khr_fp16
whether the extension is available or not, right?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 understand Ultima can solve the problem.
But I don't understand why you need Ultima. As I told you in the some old issue, you tend to solve everything by using Ultima but we must avoid it unless there's no solution without Ultima. Minimize Ultima to minimize maintenance difficulties.
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.
Actually, I need the changes in this PR to ultima only for passing
ultima_tests
.If we decide to change the behavior of
half
identifier, we doesn't need it.However, we will need it to support all
half
operations, so I proposed this changes.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.
As of #266 (comment), no
half
variable will be constructed by users. So you don't need Ultima.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.
Until to decide to support all
half
operations . OK.Then, when you
? Again, we need to decide to change the behavior of
half
identifier from current ClPy to disablecl_khr_fp16
without Ultima. It means that we need to fix or disable some tests. If you have already decided it in your own, you should indicate your opinion here.