[tests] test_conv_hip_igemm_xdlops to gTest #2292
Conversation
|
@xinlipn please resolve conflicts and this PR has passed CI once. |
test/gtest/conv_hip_igemm_xdlops.cpp
Outdated
| case miopenInt8: params = ConfigWithInt8::GetParam(); break; | ||
| case miopenHalf: | ||
| case miopenBFloat16: | ||
| case miopenFloat: | ||
| case miopenInt8x4: | ||
| case miopenInt32: | ||
| case miopenDouble: | ||
| FAIL() << "miopenHalf, miopenBFloat16, miopenFloat, miopenInt8x4, miopenInt32, " | ||
| "miopenDouble data " | ||
| "type not supported by " | ||
| "conv_hip_igemm_mlir_xdlops test"; |
There was a problem hiding this comment.
[Recommendation] At the time of adding the ConvHipImplicitGemmFwdXdlops solver (in #1692), only the INT8 type was supported, and the initial test set was of INT8 type only. Then there was #1803 which adds support for HALF and FLOAT, but that PR is missing the support of these new datatypes in tests. That is why the test lacks HALF and FLOAT support.
There is another problem with this test: it allows all solvers to work and therefore there there is no guarantee that namely the ConvHipImplicitGemmFwdXdlops is being tested. If there is a problem with it (i.e. it is disabled), then any other solver (i.e. Gemm) can do the job and test would succeed.
@xinlipn @JehandadKhan What about resolving there problems at once? Or let's address these in a separate PR?
There was a problem hiding this comment.
[Recommendation] At the time of adding the ConvHipImplicitGemmFwdXdlops solver (in #1692), only the INT8 type was supported, and the initial test set was of INT8 type only. Then there was #1803 which adds support for HALF and FLOAT, but that PR is missing the support of these new datatypes in tests. That is why the test lacks HALF and FLOAT support.
There is another problem with this test: it allows all solvers to work and therefore there there is no guarantee that namely the
ConvHipImplicitGemmFwdXdlopsis being tested. If there is a problem with it (i.e. it is disabled), then any other solver (i.e. Gemm) can do the job and test would succeed.@xinlipn @JehandadKhan What about resolving there problems at once? Or let's address these in a separate PR?
@atamazov , thanks for bringing up these points. Does it make sense to open another PR? The reason is 1) Please correct me if I was wrong, it seems ConvHipImplicitGemmFwdXdlops is a different test, we can update with the datatypes you mentioned in its own gTest 2) in case it's the same test as here, this PR can be used as 1:1 map from the original cTest and any other changes can be put in a separate PR to avoid confusion.
There was a problem hiding this comment.
- Please correct me if I was wrong, it seems ConvHipImplicitGemmFwdXdlops is a different test
This test_conv_hip_igemm_xdlops (which is being refactored in this PR) was introduced together with the ConvHipImplicitGemmFwdXdlops solver, in #1692 (which was created by @JehandadKhan). Therefore I suspect that this test is intended to test specifically ConvHipImplicitGemmFwdXdlops. If this is so, then I recommend adding support for other datatypes, and limiting it to the ConvHipImplicitGemmFwdXdlops only.
Please discuss with @JehandadKhan in order to clarify things and to decide what needs to be done. My comment is only recommendation.
- in case it's the same test as here, this PR can be used as 1:1 map from the original cTest and any other changes can be put in a separate PR to avoid confusion.
No objections!
atamazov
left a comment
atamazov
left a comment
There was a problem hiding this comment.
LGTM. I am assuming that #2292 (comment) will be followed up here or in the separate PR.
I think that makes sense, @xinlipn can address the comment in a follow up PR. |
|
Tested on gfx908, [email protected] $printenv | grep MIOPEN gtest passed, log is as below: |
Convert ctest test_conv_hip_igemm_xdlops to gTest