-
-
Notifications
You must be signed in to change notification settings - Fork 190
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
Add kernel generator cast operation #2472
Conversation
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
This is still waiting for a review. |
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 the code looks good but I'm having a hard time following how the cast happens on the device side. I think for this operation in particular where we are sort of fighting a thing in the OpenCL spec it's very good to have more particular docs. If you can clear that up then I think this is good to go
Side note, not necessary for this PR, but I think this is an example where it would be nice to have something in the tests or an easy scheme available from runTests.py
for generating the OpenCL kernel code. It would also make reviewing these things a lot simpler. Sort of like how in the stanc3 compiler we have the generated cpp as part of the testing so we can see what stuff changes in the gen'd code between PRs
res.body = type_str<Scalar>() + " " + var_name_ + " = (" | ||
+ type_str<Scalar>() + ")" + var_name_arg + ";\n"; |
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.
So this line here generates Scalar var_name = (Scalar)input_var_name;
? If so then idt I'm understanding how this works for char
?
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.
Yes. I don't understand what is the confusion with char. It works exactly the same as for any other supported scalar type.
Side note, not necessary for this PR, but I think this is an example where it would be nice to have something in the tests or an easy scheme available from runTests.py for generating the OpenCL kernel code.
We do have reference kernels in tests, although I am not using them for operations that generate really trivial code, such as typecasting in this PR.
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.
So I'm looking at
auto any_n_negative = colwise_max(cast<char>(n < 0));
And here this is turned into
char var_name = (char)input_var_name;
Apologies if this is silly but I'm not following how that is used in an if etc. Like is it that if input_var_name
is 0 then the char
is 0 and the if is false?
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.
Yes, exactly. input_var_name
is in this case the result of the comparison n < 0
, which is either 0 or 1. 0 or 1 converted into char is still 0 or 1. That gets written into global memory and than copied to host. Is there still anything you are not understanding?
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.
Aight then I think that makes sense
@SteveBronder This PR is still waiting. |
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.
It would be good to have a note on the behavior for this but I think it is fine as is
Summary
Adds typecast operation to kernel generator and uses it in distributions instead of forcing type promotions by adding zero.
Tests
Added tests for cast. Reminder of this PR is just refactor.
Side Effects
None.
Release notes
Adds typecast operation to kernel generator.
Checklist
Math issue #(issue number)
Copyright holder: Tadej Ciglarič
The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
./runTests.py test/unit
)make test-headers
)make test-math-dependencies
)make doxygen
)make cpplint
)the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested