[Profiling] Update elementwise op #6229
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chengduoZH
changed the title
chengduoZH
force-pushed
the
profiling/updata_elementwise_op
branch
from
2a5fb74
to
ff4c825
Compare
chengduoZH
force-pushed
the
profiling/updata_elementwise_op
branch
3 times, most recently
from
18a50a2
to
cc210de
Compare
chengduoZH
force-pushed
the
profiling/updata_elementwise_op
branch
from
cc210de
to
54f0962
Compare
reyoung
reviewed
Dec 5, 2017
| : ptr_(ptr), i_(0), j_(0), n_(n), post_(post) {} | ||
|
|
||
| MidWiseTransformIterator<T, platform::CPUPlace>& operator++() { | ||
| i_ = ++j_ / post_ % n_; |
There was a problem hiding this comment.
Please add parentheses here.
|
|
||
| bool operator!=( | ||
| const MidWiseTransformIterator<T, platform::CPUPlace>& rhs) const { | ||
| return (ptr_ + i_) &= &(*rhs); |
| typedef thrust::iterator_adaptor< | ||
| RowwiseTransformIterator<T, platform::GPUPlace>, const T*> | ||
| super_t; | ||
| __host__ __device__ RowwiseTransformIterator(const T* x, int n) |
There was a problem hiding this comment.
Please use HOSTDEVICE macro
| private: | ||
| unsigned int n_; | ||
| const T* begin_; | ||
| __host__ __device__ typename super_t::reference dereference() const { |
There was a problem hiding this comment.
Please use HOSTDEVICE macro
| template <typename Place, typename T> | ||
| class ElementwiseAddKernel : public framework::OpKernel<T> { | ||
| public: | ||
| void Compute(const framework::ExecutionContext& ctx) const override { | ||
| ElementwiseCompute<EigenAddFunctor, Place, T>(ctx); | ||
| using Tensor = framework::Tensor; |
There was a problem hiding this comment.
Maybe we should implement all elemwise operators based on this method.
Not in hurry, but need an issue to record the following works.
There was a problem hiding this comment.
We can do these in next PR.
QiJune
reviewed
Dec 5, 2017
| template <typename Functor, typename T, typename Place> | ||
| struct TransformFunctor { | ||
| TransformFunctor(const framework::Tensor* x, const framework::Tensor* y, | ||
| framework::Tensor* z, const framework::ExecutionContext& ctx, |
There was a problem hiding this comment.
Better to use DeviceContext other than ExecutionContext
chengduoZH
force-pushed
the
profiling/updata_elementwise_op
branch
3 times, most recently
from
830dbaa
to
1517889
Compare
chengduoZH
force-pushed
the
profiling/updata_elementwise_op
branch
from
1517889
to
9e244a8
Compare
qingqing01
reviewed
Dec 5, 2017
| T* z_; | ||
| int64_t nx_; | ||
| const platform::DeviceContext& ctx_; | ||
| Functor func_; |
There was a problem hiding this comment.
The naming rules of data members for struct is different from class.
https://google.github.io/styleguide/cppguide.html#Variable_Names
There was a problem hiding this comment.
Done, I have replaced struct with class.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fix #6166
"We found the performance of
Eigen::Tensor::broadcastis not good if theTensoris not just a scalar, because theEigen::Tensor::broadcasttry to implement a very generalbroadcastmethod. However, the most ofbroadcastoperators in the neural network isrowwiseorcolwise. The implementation ofrowwiseorcolwiseis much simpler and faster than a general implementation.The
Eigen::Tensor::broadcastis mainly implmeneted by this function." —— @reyoung