[gpu/blas] sgemv_noTrans kernel and unit tests @open sesame 10/29 21:54 #2770

s-debadri · 2024-10-24T07:14:48Z

Changes in this PR:

Added sgemv_noTrans kernel.
trans information was added to the required functions.
Added all possible unit tests for sgemv and sgemv_noTrans.

Self evaluation:

Build test: [X]Passed [ ]Failed [ ]Skipped
Run test: [X]Passed [ ]Failed [ ]Skipped

Signed-off-by: Debadri Samaddar [email protected]

taos-ci · 2024-10-24T07:14:51Z

📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #2770. 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/.

skykongkong8 · 2024-10-24T07:19:21Z

test/unittest/unittest_blas_kernels_cl.cpp

+  nntrainer::TensorDim::TensorType t_type_nchw_fp16 = {
+    nntrainer::Tformat::NCHW, nntrainer::Tdatatype::FP16};
+


Remove? (this is fp32 TC so it is not needed)

And I wonder why there is no fp16 TCs as well.

Removed unused variables. Also we are focussing on accuracy as of now. Will add fp16 tests with later PRs.

Added kernel for sgemv_noTrans. Modified the conditions of blas_kernel_interface. Signed-off-by: Debadri Samaddar <[email protected]>

taos-ci

@s-debadri, 💯 All CI checkers are successfully verified. Thanks.

djeong20

Overall, LGTM :)
Please check the comments!

djeong20 · 2024-10-29T01:35:11Z

nntrainer/tensor/cl_operations/blas_kernel_interface.cpp

+      trans ? sgemv_cl(data, mdata, rdata, trans, dim2, dim1, lda)
+            : sgemv_cl(data, mdata, rdata, trans, dim1, dim2, lda);


One suggestion!
Since trans is now added as a parameter, handling dim1 and dim2 in the sgemv_cl function would be possible.

Suggested change

trans ? sgemv_cl(data, mdata, rdata, trans, dim2, dim1, lda)

: sgemv_cl(data, mdata, rdata, trans, dim1, dim2, lda);

sgemv_cl(data, mdata, rdata, trans, dim2, dim1, lda)

and

// sgemv_cl size_t dim1_size = transA ? sizeof(float) * dim2 : sizeof(float) * dim1; size_t dim2_size = transA ? sizeof(float) * dim1 : sizeof(float) * dim2;

The calls for N == 1 and M == 1 are a bit different. Also the work group size depends on the dim1 and dim2. I think handling it before the function call would be better so that we can have a generalized function.

nntrainer/cl_context.cpp

DonghakPark

Overall, it is good.
Thank you for considering test cases as well as FP32 and FP16 cases in your work. 👍

DonghakPark · 2024-10-29T02:17:09Z

test/unittest/unittest_blas_kernels_cl.cpp

@@ -32,13 +32,58 @@ static void setUpGpuContext() {
  ac.initBlasClKernels();
 }

-TEST(blas_kernels, dotCL_sgemv) {
+TEST(blas_kernels, dotCL_sgemv_M_1_1) {


One more Question)
I brought this PR to my local environment for testing.

It seems that even when setting the enable-opencl option to true, cannot reach the corresponding unit test.
Is it correct that the current test file is not included in any of the meson.build files within the unittest folder?

And when add

if get_option('enable-opencl') test_target += [['unittest_blas_kernels_cl', []]] endif

this to unittest/meson.build

i got this build error

/home/donghak/Desktop/workspace/nntrainer/nntrainer/opencl/opencl_program.cpp:73:10: error: variable length array ‘binaries_size’ is used [-Werror=vla] 73 | size_t binaries_size[num_devices]; | ^~~~~~~~~~~~~ /home/donghak/Desktop/workspace/nntrainer/nntrainer/opencl/opencl_program.cpp:98:8: error: variable length array ‘kernel_names’ is used [-Werror=vla] 98 | char kernel_names[kernel_names_size]; | ^~~~~~~~~~~~ /home/donghak/Desktop/workspace/nntrainer/nntrainer/opencl/opencl_program.cpp:113:18: error: variable length array ‘binaries_ptr’ is used [-Werror=vla] 113 | unsigned char *binaries_ptr[num_devices];

This error should be fixed to enable x86 OpenCL compilation (for tizen build as well). This error may not be detected in Android build. One can face this error, for compilation with PC environment doesn't support dynamic allocation like that. I will make an issue for it :
#2776

unittest_blas_kernels_cl was written for Android tests. So, it's only present in the Android.mk file.

@DonghakPark There was an error in my previous comment. The PR #2727 resolves this issue! But it needs to be rebased. I will update the issue as well. Sorry for confusion

I understand. Thank you for your explanations! 👍

taos-ci

@s-debadri, 💯 All CI checkers are successfully verified. Thanks.

s-debadri 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 October 24, 2024 07:14

github-actions bot added the Need Review label Oct 24, 2024

skykongkong8 reviewed Oct 24, 2024

View reviewed changes

[gpu/blas] Implemented sgemv_noTrans scenario

3a19748

Added kernel for sgemv_noTrans. Modified the conditions of blas_kernel_interface. Signed-off-by: Debadri Samaddar <[email protected]>

s-debadri force-pushed the sgemv_cl_trans branch from 2381a17 to 3a19748 Compare October 24, 2024 07:27

taos-ci approved these changes Oct 24, 2024

View reviewed changes

djeong20 approved these changes Oct 29, 2024

View reviewed changes

DonghakPark reviewed Oct 29, 2024

View reviewed changes

nntrainer/cl_context.cpp Show resolved Hide resolved

DonghakPark reviewed Oct 29, 2024

View reviewed changes

nntrainer/cl_context.cpp Show resolved Hide resolved

DonghakPark approved these changes Oct 29, 2024

View reviewed changes

github-actions bot added PR/READY2MERGE and removed Need Review labels Oct 29, 2024

DonghakPark reviewed Oct 29, 2024

View reviewed changes

EunjuYang mentioned this pull request Oct 29, 2024

[ GPU ] Enable x86 OpenCL with some fixes #2776

Open

jijoongmoon changed the title ~~[gpu/blas] sgemv_noTrans kernel and unit tests~~ [gpu/blas] sgemv_noTrans kernel and unit tests @open sesame 10/29 21:54 Oct 29, 2024

taos-ci approved these changes Oct 29, 2024

View reviewed changes

skykongkong8 approved these changes Nov 11, 2024

View reviewed changes

jijoongmoon merged commit 9af6680 into nnstreamer:main Nov 14, 2024
59 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[gpu/blas] sgemv_noTrans kernel and unit tests @open sesame 10/29 21:54 #2770

[gpu/blas] sgemv_noTrans kernel and unit tests @open sesame 10/29 21:54 #2770

s-debadri commented Oct 24, 2024

taos-ci commented Oct 24, 2024

skykongkong8 Oct 24, 2024 •

edited

Loading

skykongkong8 Oct 24, 2024

s-debadri Oct 28, 2024

taos-ci left a comment

djeong20 left a comment

djeong20 Oct 29, 2024

djeong20 Oct 29, 2024

s-debadri Oct 29, 2024

DonghakPark left a comment

DonghakPark Oct 29, 2024

DonghakPark Oct 29, 2024

EunjuYang Oct 29, 2024

s-debadri Oct 29, 2024

EunjuYang Oct 29, 2024

DonghakPark Oct 30, 2024

taos-ci left a comment

		nntrainer::TensorDim::TensorType t_type_nchw_fp16 = {
		nntrainer::Tformat::NCHW, nntrainer::Tdatatype::FP16};

		trans ? sgemv_cl(data, mdata, rdata, trans, dim2, dim1, lda)
		: sgemv_cl(data, mdata, rdata, trans, dim1, dim2, lda);

	trans ? sgemv_cl(data, mdata, rdata, trans, dim2, dim1, lda)
	: sgemv_cl(data, mdata, rdata, trans, dim1, dim2, lda);
	sgemv_cl(data, mdata, rdata, trans, dim2, dim1, lda)

[gpu/blas] sgemv_noTrans kernel and unit tests @open sesame 10/29 21:54 #2770

[gpu/blas] sgemv_noTrans kernel and unit tests @open sesame 10/29 21:54 #2770

Conversation

s-debadri commented Oct 24, 2024

taos-ci commented Oct 24, 2024

skykongkong8 Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taos-ci left a comment

Choose a reason for hiding this comment

djeong20 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonghakPark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taos-ci left a comment

Choose a reason for hiding this comment

skykongkong8 Oct 24, 2024 •

edited

Loading