-
-
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 broadcasting to kernel generator #1797
Add broadcasting to kernel generator #1797
Conversation
…xp1~20180509124008.99 (branches/release_50)
…stable/2017-11-14)
…stable/2017-11-14)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
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.
Nice! Couple lil' comments and one API question
inline kernel_parts generate(const std::string& i, const std::string& j, | ||
const std::string& var_name_arg) const { |
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.
[seperate pr]
Reading this over wrt modify_argument_indices
it's kind of confusing to read i
and j
here as the names of the index but in modify_argument_indices
as the actual index values. It may be good here to change the argument names to something like i_name
and j_name
. Or maybe like row_idx_name
and col_idx_name
Co-Authored-By: Steve Bronder <Stevo15025@gmail.com>
…xp1~20180509124008.99 (branches/release_50)
…tatcomp/math into cl_kernel_generator_broadcast
…gs/RELEASE_500/final)
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
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.
One doc thing that would be good to change since I figure we'll forget later but I'll approve now. If you make the change I'll re-approve rq
} | ||
|
||
/** | ||
* Brodcast an expression in colwise dimmension. The argument must have single |
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.
Can you just find/replace Bordcast
with Broadcast
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.
lgtm!
Jenkins Console Log Machine informationProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010CPU: G++: Clang: |
Summary
Adds broadcasting to kernel generator. If an expression is broadcasted in a dimmension it can be used as if it has any size in that dimmension when used in an operation that accepts more than one argument. The size is determined from other arguments to same expression.
Also contains a bugfix for determining view of the result.
Tests
Added tests for broadcasting and bug.
Side Effects
None.
Release notes
Added broadcasting to kernel generator.
Checklist
Math issue Implement OpenCL kernel generator #1342
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