complex gemm implementation by TonyYHsieh · Pull Request #626 · ROCm/rocBLAS

TonyYHsieh · 2019-07-30T10:25:21Z

complex gemm(_ex) and gemm_strided_batched(ex) implementation
rocblas test and benchmark for complex

resolves #___

Summary of proposed changes:

leekillough · 2019-07-31T22:03:47Z

rocblas_isnan() should be applied to arg.beta rather than arg.get_beta<T>().

Understand, I correct it on third commit "update complex change".
But, do we have to check nan for betai too??
It is only used in complex type.
Should we use another template function for is_nan check??

is_nan should be defined for rocblas_float_complex and rocblas_double_complex, which will return true if either the real or imaginary part is NaN.

But since arg.beta and arg.betai are always floating-point types, you can use std::isnan(arg.beta) || std::isnan(arg.betai).

understand. update it in commit "update complex change 2".

leekillough · 2019-07-31T22:04:59Z

arg.get_beta<T>() should already consider whether rocblas_isnan(arg.beta) and return accordingly.

understand. This is my mistake.
Update it on third commit "update complex change"

leekillough · 2019-07-31T22:05:48Z

rocblas_isnan(arg.beta) should be left alone -- it is separate from arg.get_beta<T>().

Understand, I will correct it on third commit "update complex change".
But, do we have to check nan for betai too??
It is only used in complex type.
Should we use another template function for is_nan check??

rocblas_isnan(arg.beta) || rocblas_isnan(arg.betai). It does not matter that betai is only used during complex. It will default to 0 in non-complex cases.

understand. update it in commit "update complex change 2".

whchung · 2019-07-31T22:08:18Z

+@deven-amd +@jerryyin for awareness. CGEMM / ZGEMM in rocBLAS.

leekillough · 2019-07-31T22:09:17Z

We might want a trailing comma, for consistency

Understand.
I correct it on third commit "update complex change".

leekillough · 2019-07-31T22:23:37Z

std::is_same<T1,T2>{} can be used as shorthand instead of std::is_same<T1,T2>::value, but this is not critical.

Understand, I correct it on third commit "update complex change".

leekillough · 2019-07-31T22:25:18Z

unsigned(...) can be used instead of static_cast<unsigned int>(...) for brevity.

Understand,
I correct it on third commit "update complex change".

leekillough

Ignore the CI build failures, which are global. Saad is working on a fix for them.

I have verified that the full rocBLAS tests pass on GFX900 and GFX906.

I will look at the code some more, but I approve the changes, as long as:

Complex values for alpha and beta are tested sufficiently.
Complex values for A, B, C, D matrices are tested sufficiently.
Most of the concerns I've raised in comments are addressed or downplayed, such as the isnan stuff with beta. Some of the suggestions can be addressed in a later refactoring change.

amcamd

Need to review alpha and beta behavior when they have NaN values

TonyYHsieh · 2019-08-06T06:57:36Z

Need to review alpha and beta behavior when they have NaN values

Library side : didn’t treat NaN as a special case, so NaN operate with other value will generate NaN.
This implementation looks same as cblas implementation.

Client side: set C as NaN if beta is NaN and set beta as 0 when beta is NaN
I follow previous NaN implementation when implement complex gemm.
So what is the expectation about NaN implementation??

TonyYHsieh · 2019-08-06T06:59:10Z

Ignore the CI build failures, which are global. Saad is working on a fix for them.

I have verified that the full rocBLAS tests pass on GFX900 and GFX906.

I will look at the code some more, but I approve the changes, as long as:

Lee : 1. Complex values for alpha and beta are tested sufficiently.

Reply :We have add alpha and beta imaginary value in rocblas-test for complex gemm
Which matrix size and other parameter is same real type.
Do we need more other testing for complex gemm??

Lee: 2. Complex values for A, B, C, D matrices are tested sufficiently.

Reply: Same as item a. We have add complex test item in rocblas-test.
Which matrix size and other parameters is same real type.
All test item is pass.

Lee: 3. Most of the concerns I've raised in comments are addressed or downplayed, such as the isnan stuff with beta. Some of the suggestions can be addressed in a later refactoring change.

Reply :We have modified implementation according to your comment and update in other commits in this PR (do we need to squash all commits to one commit ??).
We only met issue on one comment. ”bfloat16 to double conversion”
We met build error when convert bfloat16 to double if we didn’t add double conversion.
So we have to add double conversion. Compile didn’t convert bfloat16 to float and to double when double(bfloat16{1.0}).

leekillough · 2019-08-06T19:21:12Z

I would remove comments about "Complex is not supported yet".

understand. update it in commit "update complex change 2".

leekillough · 2019-08-06T19:28:32Z

We use C++14 for our internal code (not external C interfaces), so it's okay to use auto as a placeholder for function return type.

understand. update it in commit "update complex change 2".

leekillough · 2019-08-06T19:30:17Z

TODO: Consolidate these functions into a smaller number of templates , varying them only by template arguments.

Need to wrap rocblas_half to a class form to get conversion operation from half to double.
I think this can be done later.

leekillough · 2019-08-06T19:31:41Z

Pointers to void are not allowed as template arguments. It should be changed to:
template <typename T, typename std::enable_if<!is_complex<T>, int>::type = 0>

understand. update it in commit "update complex change 2".

leekillough · 2019-08-06T19:32:49Z

As above:
template <typename T, typename std::enable_if<!is_complex<T>, int>::type = 0>

understand. update it in commit "update complex change 2".

leekillough · 2019-08-06T19:37:12Z

This may need to be changed to:

bool nantest = rocblas_isnan(arg.beta) || rocblas_isnan(arg.betai);

For non-complex types, betai is 0 and so rocblas_isnan(arg.betai) is false.

understand. update it in commit "update complex change 2".

leekillough · 2019-08-06T19:52:14Z

The comment is a little off.

understand. update it in commit "update complex change 2".

leekillough · 2019-08-06T20:00:02Z

Typo -- COPMLEX should be COMPLEX

understand. update it in commit "update complex change 2".

leekillough · 2019-08-06T20:00:44Z

Same typo throughout file -- maybe do global search and replacce

understand. update it in commit "update complex change 2".

leekillough · 2019-08-06T20:03:54Z

I agree with moving norm.cpp to a header file norm.hpp.

Thanks for agreement :)

leekillough

Once the changes I commented on are made, and the CI tests pass, I approve the changes.

amdkila · 2019-08-07T17:01:31Z

@TonyYHsieh Your PR failed CI due to minor formatting differences on just 1 file (rocblas_gemm_ex.cpp), so I re-ran the formatter. It should pass the format check now.

1. complex gemm(_ex) and gemm_strided_batched(ex) implementation 2. rocblas test and benchmark for complex

1. fix typo 2. fix beta nan condition check 3. fix invalid style of template function 4. fix bench log 5. add more alpha/beta setting for complex rocblas-test

TonyYHsieh requested review from amcamd and leekillough as code owners July 30, 2019 10:25

TonyYHsieh requested review from imcarsonliao and jichangjichang July 30, 2019 10:26

amcamd reviewed Jul 30, 2019

View reviewed changes

Comment thread library/include/rocblas_bfloat16.h Outdated

amcamd reviewed Jul 30, 2019

View reviewed changes

Comment thread library/include/rocblas-functions.h Outdated

amcamd requested a review from daineAMD July 30, 2019 15:14

leekillough reviewed Jul 31, 2019

View reviewed changes

Comment thread clients/include/testing_gemm.hpp Outdated

leekillough reviewed Jul 31, 2019

View reviewed changes

Comment thread library/include/rocblas_bfloat16.h Outdated

leekillough reviewed Jul 31, 2019

View reviewed changes

leekillough reviewed Aug 4, 2019

View reviewed changes

amcamd approved these changes Aug 5, 2019

View reviewed changes

amcamd requested changes Aug 5, 2019

View reviewed changes

leekillough reviewed Aug 6, 2019

View reviewed changes

Comment thread clients/gtest/gemm_gtest.yaml Outdated

leekillough reviewed Aug 6, 2019

View reviewed changes

Comment thread clients/include/testing_gemm.hpp Outdated

leekillough reviewed Aug 6, 2019

View reviewed changes

leekillough approved these changes Aug 6, 2019

View reviewed changes

TonyYHsieh and others added 8 commits August 8, 2019 18:45

complex gemm implementation

1dc13a5

1. complex gemm(_ex) and gemm_strided_batched(ex) implementation 2. rocblas test and benchmark for complex

add document for gemm_ex and correct func format

5b6e147

update complex change

dc21c4a

follow rocblas conversion guideline

7d946b9

update tensile complex logic file for massage

379ca25

update complex change 2

aa73a14

1. fix typo 2. fix beta nan condition check 3. fix invalid style of template function 4. fix bench log 5. add more alpha/beta setting for complex rocblas-test

Ran clang-format-9 on repo after it failed format check

67231cb

add complex gemm logic file to asm_ci

24df5aa

amcamd approved these changes Aug 8, 2019

View reviewed changes

TonyYHsieh closed this Aug 9, 2019

TonyYHsieh mentioned this pull request Aug 9, 2019

complex gemm implementation #643

Merged

mlse-lib-jenkins pushed a commit that referenced this pull request Apr 14, 2021

Improved boost emulation (#626)

0a57a66

Conversation

TonyYHsieh commented Jul 30, 2019

Summary of proposed changes:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TonyYHsieh Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TonyYHsieh Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TonyYHsieh Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

whchung commented Jul 31, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TonyYHsieh Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TonyYHsieh Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TonyYHsieh Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leekillough left a comment

Choose a reason for hiding this comment

Uh oh!

amcamd left a comment

Choose a reason for hiding this comment

Uh oh!

TonyYHsieh commented Aug 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TonyYHsieh commented Aug 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

TonyYHsieh Aug 1, 2019 •

edited

Loading

TonyYHsieh Aug 1, 2019 •

edited

Loading

TonyYHsieh Aug 1, 2019 •

edited

Loading

TonyYHsieh Aug 1, 2019 •

edited

Loading

TonyYHsieh Aug 1, 2019 •

edited

Loading

TonyYHsieh Aug 1, 2019 •

edited

Loading

TonyYHsieh commented Aug 6, 2019 •

edited

Loading

TonyYHsieh commented Aug 6, 2019 •

edited

Loading