[ Tensor ] Remove CBLAS params from Tensor related files. #2704

skykongkong8 · 2024-08-12T04:21:25Z

Remove cblas params from tensor related files since nntrainer is not fully-dependent on cblas anymore.
Letting tensors to be aware of Cblas related parameters is a nonsense at the first place.
CBLAS params will be declared only when functions from cblas is called.
fyi) TStorageOrder : Tensor Storage Order

Self evaluation:

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

taos-ci · 2024-08-12T04:21:27Z

📝 TAOS-CI Version: 1.5.20200925. Thank you for submitting PR #2704. 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-08-12T04:21:45Z

This pr is related to known issue raised from #2682

taos-ci · 2024-08-12T05:31:34Z

cibot: @skykongkong8, A builder checker could not be completed because one of the checkers is not completed. In order to find out a reason, please go to http://ci.nnstreamer.ai/nntrainer/ci/repo-workers/pr-checker/2704-202408121359440.67014408111572-ff8acfbbcb564ab957d91ca5670e01165424da05/.

- Remove cblas params from tensor related files since nntrainer is not fully-dependent on cblas anymore. - Letting tensors to be aware of Cblas related parameters is a nonsense at the first place. - CBLAS params will be declared only when functions from cblas is called. **Self evaluation:** 1. Build test: [X]Passed [ ]Failed [ ]Skipped 2. Run test: [X]Passed [ ]Failed [ ]Skipped Signed-off-by: skykongkong8 <[email protected]>

skykongkong8 · 2024-08-12T07:23:08Z

@s-debadri
Please do not let cblas params to reside outside of actual cblas function interface.
I could observe some from opencl related works.

taos-ci

@skykongkong8, 💯 All CI checkers are successfully verified. Thanks.

nntrainer/tensor/cl_operations/blas_kernel_interface.cpp

djeong20

Great work! please take a look at the comments :)

djeong20 · 2024-08-19T06:42:47Z

nntrainer/tensor/blas_interface.cpp

@@ -93,8 +99,7 @@ static inline void transpose_fallback(unsigned int M, unsigned int N,
 static void saxpy_FP16(const unsigned int N, const float alpha, const _FP16 *X,
                       const int incX, _FP16 *Y, const int incY) {
  if (incX < 0 or incY < 0)
-    throw std::invalid_argument(
-      "Error: negative inc not supported without cblas");
+    throw std::invalid_argument("Error: negative inc not supported");


Q1) is negative increment always not supported?
Q2) what happens when the increment is zero?

incX and incY are indices, thus should be always positive. I think this would answer both questions!

djeong20 · 2024-08-19T06:53:59Z

nntrainer/tensor/blas_interface.cpp

-    cublasOperation_t transB =
-      (TransB == CblasTrans) ? CUBLAS_OP_T : CUBLAS_OP_N;
+    cublasOperation_t transA = (TransA) ? CUBLAS_OP_T : CUBLAS_OP_N;
+    cublasOperation_t transB = (TransB) ? CUBLAS_OP_T : CUBLAS_OP_N;
    cublasSgemm(handle, transA, transB, N, M, K, &alpha, d_B, N, d_A, K, &beta,


it looks like cuBLAS interprets matrices as column-major. we should preprocess (e.g., transpose) to correctly use cublasSgemm. For now, let's mark it as ToDo.

Never knew it 😮 Thanks for pointing this out!

Indeed, it does take matrices as Col-Maj storage order !
https://stackoverflow.com/questions/56043539/cublassgemm-row-major-multiplication

djeong20 · 2024-08-19T07:08:08Z

nntrainer/tensor/float_tensor.cpp

@@ -493,8 +493,8 @@ void FloatTensor::sum_by_batch(Tensor &output) const {

  Tensor ones(1, 1, 1, feat_len, this->getFormat());
  ones.setValue(1.0);
-  sgemv(CblasRowMajor, CblasNoTrans, batch, feat_len, 1, data, feat_len,
-        ones.getData<float>(), 1, 0.0, out_data, 1);
+  sgemv((unsigned int)dim.getStorageOrder(), false, batch, feat_len, 1, data,


This is just a suggestion! how about having a fixed value for storage orders like transpose?
although there's no difference in the result, I think it would be easier for us to understand the code and debug.

I don't really get it... Could you elaborate a little bit more for me?
I think current implementation is quite similar to transpose cases.
With my understanding of your suggestion, do you mean we should have functions like:

sgemv_rowMaj(...) ... sgemv_colMah(...) ....

?

what I meant by having a fixed value is as follows.

Suggested change

sgemv((unsigned int)dim.getStorageOrder(), false, batch, feat_len, 1, data,

sgemv(TStorageOrder::ROW_MAJOR, false, batch, feat_len, 1, data,

same as we pass the transpose with true/false!

That's a good one!

This PR resolves the build error after nnstreamer#2704 when enable_fp16 is true. This fixes: blas_interface.cpp:141:9: error: ‘order’ was not declared in this scope 141 | sgemv(order, TransA, M, N, alpha, A_, lda, X_, incX, beta, Y_, incY); | ^~~~~ **Self-evaluation:** 1. Build test: [X]Passed [ ]Failed [ ]Skipped 2. Run test: [ ]Passed [X]Failed [ ]Skipped Signed-off-by: Donghyeon Jeong <[email protected]>

This PR resolves the build error after #2704 when enable_fp16 is true. This fixes: blas_interface.cpp:141:9: error: ‘order’ was not declared in this scope 141 | sgemv(order, TransA, M, N, alpha, A_, lda, X_, incX, beta, Y_, incY); | ^~~~~ **Self-evaluation:** 1. Build test: [X]Passed [ ]Failed [ ]Skipped 2. Run test: [ ]Passed [X]Failed [ ]Skipped Signed-off-by: Donghyeon Jeong <[email protected]>

skykongkong8 requested review from myungjoo, jijoongmoon, again4you, jaeyun-jung, leemgs, wooksong, helloahn, kparichay, gichan-jang, anyj0527 and zhoonit as code owners August 12, 2024 04:21

skykongkong8 requested review from lhs8928, songgot, jihochu, DonghakPark, SeoHyungjun, baek2sm, djeong20, EunjuYang and a team as code owners August 12, 2024 04:21

github-actions bot added the Need Review label Aug 12, 2024

skykongkong8 linked an issue Aug 12, 2024 that may be closed by this pull request

Remove Cblas params from tensor related files #2682

Closed

skykongkong8 force-pushed the pr/tensor/rm_cblas_params branch 2 times, most recently from 6b51401 to ff8acfb Compare August 12, 2024 04:59

skykongkong8 force-pushed the pr/tensor/rm_cblas_params branch from ff8acfb to 997beb0 Compare August 12, 2024 06:56

skykongkong8 force-pushed the pr/tensor/rm_cblas_params branch from 997beb0 to c952f2f Compare August 12, 2024 06:57

taos-ci approved these changes Aug 12, 2024

View reviewed changes

SeoHyungjun approved these changes Aug 14, 2024

View reviewed changes

nntrainer/tensor/cl_operations/blas_kernel_interface.cpp Show resolved Hide resolved

djeong20 approved these changes Aug 19, 2024

View reviewed changes

github-actions bot added PR/READY2MERGE and removed Need Review labels Aug 19, 2024

baek2sm approved these changes Aug 21, 2024

View reviewed changes

jijoongmoon merged commit 6623e30 into nnstreamer:main Aug 22, 2024
48 checks passed

This was referenced Aug 23, 2024

[bugfix] Resolve fp16 enabled build error #2714

Merged

Main branch error after merging #2704 #2715

Closed

skykongkong8 deleted the pr/tensor/rm_cblas_params branch October 2, 2024 02:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ Tensor ] Remove CBLAS params from Tensor related files. #2704

[ Tensor ] Remove CBLAS params from Tensor related files. #2704

skykongkong8 commented Aug 12, 2024 •

edited

Loading

taos-ci commented Aug 12, 2024

skykongkong8 commented Aug 12, 2024 •

edited

Loading

taos-ci commented Aug 12, 2024

skykongkong8 commented Aug 12, 2024

taos-ci left a comment

djeong20 left a comment

djeong20 Aug 19, 2024

skykongkong8 Aug 19, 2024

djeong20 Aug 19, 2024

skykongkong8 Aug 19, 2024

skykongkong8 Aug 19, 2024

djeong20 Aug 19, 2024

skykongkong8 Aug 19, 2024

djeong20 Aug 20, 2024

skykongkong8 Aug 20, 2024

	sgemv((unsigned int)dim.getStorageOrder(), false, batch, feat_len, 1, data,
	sgemv(TStorageOrder::ROW_MAJOR, false, batch, feat_len, 1, data,

[ Tensor ] Remove CBLAS params from Tensor related files. #2704

[ Tensor ] Remove CBLAS params from Tensor related files. #2704

Conversation

skykongkong8 commented Aug 12, 2024 • edited Loading

taos-ci commented Aug 12, 2024

skykongkong8 commented Aug 12, 2024 • edited Loading

taos-ci commented Aug 12, 2024

skykongkong8 commented Aug 12, 2024

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

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

skykongkong8 commented Aug 12, 2024 •

edited

Loading

skykongkong8 commented Aug 12, 2024 •

edited

Loading