[GPU/OpenCL] Initial version of Transpose function with OpenCL ops @open sesame 10/04 17:03 #2690
Conversation
niket-agarwal
requested review from
myungjoo,
jijoongmoon,
again4you,
jaeyun-jung,
leemgs,
wooksong,
helloahn,
kparichay,
gichan-jang,
anyj0527,
zhoonit,
lhs8928,
songgot,
jihochu,
DonghakPark,
SeoHyungjun,
baek2sm,
skykongkong8,
djeong20,
EunjuYang and
a team
as code owners
|
📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #2690. Please a submit 1commit/1PR (one commit per one PR) policy to get comments quickly from reviewers. Your PR must pass all verificiation processes of cibot before starting a review process from reviewers. If you are new member to join this project, please read manuals in documentation folder and wiki page. In order to monitor a progress status of your PR in more detail, visit http://ci.nnstreamer.ai/. |
|
|
cibot:
|
|
|
|
niket-agarwal
force-pushed
the
Transpose
branch
from
7e976e3 to
e3010ee
Compare
|
|
|
|
niket-agarwal
force-pushed
the
Transpose
branch
from
e3010ee to
c3364f8
Compare
|
|
niket-agarwal
force-pushed
the
Transpose
branch
from
4ea3ba8 to
0456110
Compare
There was a problem hiding this comment.
@niket-agarwal, 💯 All CI checkers are successfully verified. Thanks.
niket-agarwal
force-pushed
the
Transpose
branch
2 times, most recently
from
2d5a01a to
0c7523e
Compare
|
Nice work! But.. As you already know, this transpose layer is not a general transpose layer but rather one specifically tailored for the limited environment of the llama application. As others have already mentioned in their comments, generalization would require using either the tensor::transpose or permute layers rather than the transpose layer within the llama application which only works under specific settings. |
There was a problem hiding this comment.
@niket-agarwal, 💯 All CI checkers are successfully verified. Thanks.
Whatever we have done here is also a generalized implementation only. Using transpose_i_cl which is generalised internally and can handle all the directions and would work under all settings. |
There was a problem hiding this comment.
First of all, thank you for the pull request and the hard work!
I think what @baek2sm meant is the generalization of a layer, not the kernel. I agree that transpose_i_cl() is generalized internally and can handle transpose two axes and this is a good starting point 👍. However, to explain, TransposeLayer was added only for the LLaMA application and is not used for all purposes. (we use Tensor::transpose() and PermuteLyaer instead). It is currently not used in the LLaMA application as well.
In my opinion, all works under nntrainer/tensor/cl_operations are good to go (nice work!). While TransposeLayer is not used, I think PermuteLayerCl would be a better option (maybe this could be the next PR I guess).
There was a problem hiding this comment.
Overall, LGTM! Except for the clang format :) Please check the format.
(+ Additional comment)
In MHA layer, transpose is required.
I agree with that it would be better to implement the transpose feature as a tensor operation. For this time, however, it is okay to merge this PR to enable GPU kernels to run LLM.
niket-agarwal
force-pushed
the
Transpose
branch
2 times, most recently
from
28ee6d0 to
91fed77
Compare
There was a problem hiding this comment.
@niket-agarwal, 💯 All CI checkers are successfully verified. Thanks.
niket-agarwal
force-pushed
the
Transpose
branch
from
91fed77 to
dc09f1f
Compare
…es with OpenCL ops Added naive version of OpenCL implementation for Transpose function using blas Incorporated kernels for ops used. Added unit tests for transpose about different axes. Signed-off-by: Niket Agarwal <[email protected]>
niket-agarwal
force-pushed
the
Transpose
branch
from
dc09f1f to
8f846e8
Compare
There was a problem hiding this comment.
@niket-agarwal, 💯 All CI checkers are successfully verified. Thanks.
jijoongmoon
changed the title
jijoongmoon
changed the title
There was a problem hiding this comment.
@niket-agarwal, 💯 All CI checkers are successfully verified. Thanks.
jijoongmoon
changed the title
…es with OpenCL ops Added naive version of OpenCL implementation for Transpose function using blas Incorporated kernels for ops used. Added unit tests for transpose about different axes. Signed-off-by: Niket Agarwal <[email protected]>
There was a problem hiding this comment.
@niket-agarwal, 💯 All CI checkers are successfully verified. Thanks.
…xes with OpenCL ops
Added naive version of OpenCL implementation for Transpose function using blas
Incorporated kernels for ops used.
Added unit tests for transpose about different axes.
Updating the transpose function with the recent GPU pipeline changes (blas_kernel_strings)
Signed-off-by: Niket Agarwal <[email protected]>.
niket-agarwal
force-pushed
the
Transpose
branch
from
8ea1e02 to
3f2da67
Compare
There was a problem hiding this comment.
@niket-agarwal, 💯 All CI checkers are successfully verified. Thanks.
Added initial version of Transpose function for GPU. This is a basic implementation using naive kernel.
Changes added with this PR:
Incorporated kernels for GPU to transpose about different axes.
Added
unittest_layers_transpose_cl.cppto test Transpose function on GPU.Signed-off-by: Niket Agarwal [email protected]