Tacho : compile warnings on Mi300 #13482
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_gcc
Jenkins Parameters
Build InformationTest Name: PR_gcc-openmpi_debug
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: PR_cuda
Jenkins Parameters
Build InformationTest Name: PR_intel
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
Using Repos:
Pull Request Author: iyamazaki |
|
Thank you @iyamazaki ! |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_gcc
Jenkins Parameters
Build InformationTest Name: PR_gcc-openmpi_debug
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: PR_cuda
Jenkins Parameters
Build InformationTest Name: PR_intel
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
@iyamazaki Since this code isn't tested by the autotester, I am assuming you did? Should I approve or would you like someone else to have a look? |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
ndellingwood
reviewed
Oct 1, 2024
There was a problem hiding this comment.
Hi @iyamazaki , I added suggestions to make some of the rocm version checks consistent for selecting the rocsparse_spmv* api call, keeping them consistent would help with potential changes/updates needed in the future
| @@ -1939,7 +1939,7 @@ class NumericToolsLevelSet : public NumericToolsBase<ValueType, DeviceType> { | |||
| s0.rowptrU, s0.colindU, s0.nzvalsU, | |||
| rocsparse_indextype_i32, rocsparse_indextype_i32, rocsparse_index_base_zero, rocsparse_compute_type); | |||
| // workspace | |||
| #if ROCM_VERSION >= 50400 | |||
| #if (ROCM_VERSION >= 50400 && ROCM_VERSION < 60000) | |||
There was a problem hiding this comment.
Looks like this could be made more consistent with similar changes by swapping the order of the check and function api, e.g. change L1942-1945 to be:
#if (ROCM_VERSION >= 60000)
rocsparse_spmv
#else
rocsparse_spmv_ex
| @@ -1971,7 +1975,7 @@ class NumericToolsLevelSet : public NumericToolsBase<ValueType, DeviceType> { | |||
| s0.rowptrL, s0.colindL, s0.nzvalsL, | |||
| rocsparse_indextype_i32, rocsparse_indextype_i32, rocsparse_index_base_zero, rocsparse_compute_type); | |||
| // workspace | |||
| #if ROCM_VERSION >= 50400 | |||
| #if (ROCM_VERSION >= 50400 && ROCM_VERSION < 60000) | |||
There was a problem hiding this comment.
Looks like this could be made more consistent with similar changes by swapping the order of the check and function api, e.g. change L1978-1981 to be:
#if (ROCM_VERSION >= 60000)
rocsparse_spmv
#else
rocsparse_spmv_ex
| @@ -2003,7 +2011,7 @@ class NumericToolsLevelSet : public NumericToolsBase<ValueType, DeviceType> { | |||
| s0.rowptrU, s0.colindU, s0.nzvalsU, | |||
| rocsparse_indextype_i32, rocsparse_indextype_i32, rocsparse_index_base_zero, rocsparse_compute_type); | |||
| // workspace (transpose) | |||
| #if ROCM_VERSION >= 50400 | |||
| #if (ROCM_VERSION >= 50400 && ROCM_VERSION < 60000) | |||
There was a problem hiding this comment.
Looks like this could be made more consistent with similar changes by swapping the order of the check and function api, e.g. change L2014-2017 to be:
#if (ROCM_VERSION >= 60000)
rocsparse_spmv
#else
rocsparse_spmv_ex
| @@ -2491,7 +2503,7 @@ class NumericToolsLevelSet : public NumericToolsBase<ValueType, DeviceType> { | |||
| auto vecY = ((nlvls-1-lvl)%2 == 0 ? vecW : vecL); | |||
| if (s0.spmv_explicit_transpose) { | |||
| status = | |||
| #if ROCM_VERSION >= 50400 | |||
| #if (ROCM_VERSION >= 50400 && ROCM_VERSION < 60000) | |||
There was a problem hiding this comment.
Looks like this could be made more consistent with similar changes by swapping the order of the check and function api, e.g. change to:
#if (ROCM_VERSION >= 60000)
rocsparse_spmv
#else
rocsparse_spmv_ex
| @@ -2505,7 +2517,7 @@ class NumericToolsLevelSet : public NumericToolsBase<ValueType, DeviceType> { | |||
| &buffer_size_L, (void*)buffer_L.data()); | |||
| } else { | |||
| status = | |||
| #if ROCM_VERSION >= 50400 | |||
| #if (ROCM_VERSION >= 50400 && ROCM_VERSION < 60000) | |||
There was a problem hiding this comment.
Looks like this could be made more consistent with similar changes by swapping the order of the check and function api, e.g. change to:
#if (ROCM_VERSION >= 60000)
rocsparse_spmv
#else
rocsparse_spmv_ex
| @@ -2827,7 +2839,7 @@ class NumericToolsLevelSet : public NumericToolsBase<ValueType, DeviceType> { | |||
| auto vecX = (lvl%2 == 0 ? vecU : vecW); | |||
| auto vecY = (lvl%2 == 0 ? vecW : vecU); | |||
| status = | |||
| #if ROCM_VERSION >= 50400 | |||
| #if (ROCM_VERSION >= 50400 && ROCM_VERSION < 60000) | |||
There was a problem hiding this comment.
Looks like this could be made more consistent with similar changes by swapping the order of the check and function api, e.g. change to:
#if (ROCM_VERSION >= 60000)
rocsparse_spmv
#else
rocsparse_spmv_ex
There was a problem hiding this comment.
Thank you, @ndellingwood !! Yea, my checks are ugly. I was trying to reduce the code lines.. With ROCM_VERSION >= 60000, it went back to an older naming of spmv (same as ROCM_VERSION < 50400) but with the same argument list as ROCM_VERSION >= 50400. Maybe, I'll try defining the function name based on ROCM_VERSION on the top.
ndellingwood
approved these changes
Oct 1, 2024
There was a problem hiding this comment.
@iyamazaki please confirm this still works for you in MI300 testing, thanks for the code updates!
| @@ -78,6 +78,14 @@ | |||
| #else | |||
| #define TACHO_CUSPARSE_SPMM_ALG CUSPARSE_MM_ALG_DEFAULT | |||
| #endif | |||
| #elif defined(KOKKOS_ENABLE_HIP) | |||
| #if (ROCM_VERSION >= 60000) | |||
| #define tacho_rocsparse_spmv rocsparse_spmv | |||
There was a problem hiding this comment.
That's cleaner, thanks @iyamazaki ! I'd prefer upper-case convention for macro names, but not blocking the PR on that change
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_gcc
Jenkins Parameters
Build InformationTest Name: PR_gcc-openmpi_debug
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: PR_cuda
Jenkins Parameters
Build InformationTest Name: PR_intel
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
Using Repos:
Pull Request Author: iyamazaki |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: PR_gcc-openmpi-openmp
Jenkins Parameters
Build InformationTest Name: PR_gcc
Jenkins Parameters
Build InformationTest Name: PR_gcc-openmpi_debug
Jenkins Parameters
Build InformationTest Name: PR_clang
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: PR_cuda
Jenkins Parameters
Build InformationTest Name: PR_intel
Jenkins Parameters
Build InformationTest Name: PR_cuda-uvm
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ ndellingwood ]! |
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
|
@cgcgcg sorry for missing your comment. Yes, I ran a test on Mi250 and Mi300.. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
@trilinos/shylu
Motivation
This PR removes the Tacho compile warnings seen on MI300 GPU.
Stakeholder Feedback
Testing