Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[5.5.X] hipblasDatatype_t should be replaced with HIP's hipDataType #366

Closed
emankov opened this issue Aug 18, 2021 · 4 comments · Fixed by ROCm/HIPIFY#1065
Closed

[5.5.X] hipblasDatatype_t should be replaced with HIP's hipDataType #366

emankov opened this issue Aug 18, 2021 · 4 comments · Fixed by ROCm/HIPIFY#1065
Assignees
@daineAMD

Comments

@emankov
Copy link

emankov commented Aug 18, 2021

What is the expected behavior

  1. hipDataType from HIP API should be used instead of hipblasDatatype_t.
  2. hipDataType should be populated with the corresponding hipblasDatatype_t elements, like:
    HIPBLAS_R_8I -> HIP_R_8I
  3. hipblasDatatype_t should be deleted from hipBLAS API or might be left as a typedef for hipDataType, like it is done for cublasDataType_t in cuBLAS API:
typedef cudaDataType cublasDataType_t;

[Reasons]

  1. hipblasDatatype_t being actually an analogue to cudaDataType is a type common for all HIP libraries, not only for hipBLAS, and it might be used in any HIP application.
  2. Currently, we can't just hipify cudaDataType to hipblasDatatype_t because it will require adding a corresponding #include to hipblas.h what is definitely incorrect.

What actually happens

ROCm/HIPIFY#383

How to reproduce

ROCm/HIPIFY#383

Software
hipcc --version HIP version: 4.3.0
@emankov emankov mentioned this issue Aug 18, 2021
Closed
@emankov
Copy link
Author

emankov commented Nov 24, 2021

Colleagues, any activity on the issue? It is a serious showstopper.

@emankov
Copy link
Author

emankov commented Nov 8, 2022

Still actual for ROCm HIP 5.X

@emankov emankov changed the title hipblasDatatype_t should be replaced with HIP's hipDataType [5.3.X] hipblasDatatype_t should be replaced with HIP's hipDataType Nov 8, 2022
emankov added a commit to emankov/HIPIFY that referenced this issue Nov 8, 2022
@emankov
[HIPIFY][rocBLAS][fix] Sync with rocBLAS - data types only - Part 1
c0ff7e8 
+ Continued syncing with the latest rocBLAS
+ Continued populating rocm APIs with HIP versions
+ Revised ROCm/hipBLAS#366 issue: the same goes to rocBLAS as well
@emankov emankov mentioned this issue Nov 8, 2022
Merged
@emankov emankov changed the title [5.3.X] hipblasDatatype_t should be replaced with HIP's hipDataType [5.5.X] hipblasDatatype_t should be replaced with HIP's hipDataType Feb 25, 2023
@emankov
Copy link
Author

emankov commented May 30, 2023 •
edited
Loading

@bragadeesh, we need this to be fixed ASAP. We have conflicts with HIP and hipSPARSE on hiBLAS's hipblasDatatype_t. Please use hipDataType instead, typedef hipDataType hipblasDatatype_t would be OK. The fix is very simple. Don't understand why the issue is still unresolved for almost two years.

emankov added a commit to emankov/HIPIFY that referenced this issue May 30, 2023
@emankov
[HIPIFY][ROCm#674][hipBLAS-ROCm#366][workaround] Introduce an option-…
eadd966 
…driven matcher for Data Types substitution

[Synopsis]
+ `hipSPARSE` already uses the common HIP library type `hipDataType`, whereas `hipBLAS`, `rocBLAS`, and `rocSPARSE` don't
+ So, it is needed to hipify `cudaDataType` to `hipDataType` in some cases (like for SPARSE-related sources), and to `hipblasDatatype_t`, `rocblas_datatype`, or `rocsparse_datatype` in other cases

[Solution]
+ There is no single solid solution for sources where `cudaDataType_t` is used for both SPARSE and BLAS
+ [temporary][partial] Introduce the `--use-hip-data-types` option for use `hipDataType` instead of `hipblasDatatype_t` or `rocblas_datatype`; switched off by default
+ Introduced option-driven matcher `DataTypeSelection` for Data Types substitution
+ Check the solution on `cusparseCreateSpVec` to `hipsparseCreateSpVec` hipification, where `hipDataType` is used (the synthetic test `cusparse2hipsparse.cu`)

[ToDo]
+ File tickets to `rocBLAS` and `rocSPARSE` similar to ROCm/hipBLAS#366
+ Take into account all of the supported `cudaDataType_t` enum members and update the tests
+ Remove the workaround after all the HIP libs start to use the single `hipDataType`
@emankov emankov mentioned this issue May 30, 2023
Merged
@bragadeesh
Copy link
Contributor

Adding this to @daineAMD queue.
Note that any time we have to change public interfaces, we go through a deprecation process.
This issue was blocked on all HIP datatypes to be available with all the necessary operator support. Now that those are in place, we can address it in the library for the next ROCm release.

@bragadeesh bragadeesh removed their assignment Jun 6, 2023
@emankov emankov mentioned this issue Sep 1, 2023
Closed
@emankov emankov mentioned this issue Oct 8, 2023
Closed
emankov added a commit to emankov/HIPIFY that referenced this issue Oct 13, 2023
@emankov
[HIPIFY][6.0.0][hipBLAS#366] Support for ROCm HIP 6.0.0 - Step 4 - `h…
5c080b8 
…ipblasDatatype_t` -> `hipDataType`

[IMP]
+ In hipBLAS 6.0.0, ROCm/hipBLAS#366 is finally fixed, thus HIPIFY can use `hipDataType` instead of `hipblasDatatype_t`
+ `hipDataType` can be found in `hip/library_types.h` (also mapped to CUDA's `library_types.h`)
+ `rocblas_datatype` is left unchanged

[TODO]
+ Revise all the hipBLAS functions which use `hipblasDatatype_t` or `hipDataType`
+ Remove now the unnecessary option `-use-hip-data-types`
+ Close ROCm/hipBLAS/issues/366 as implemented with the releasing of hipBLAS 6.0.0
+ [long-term] Move `hipDataType` from BLAS to Runtime, when other than hipBLAS libs will start using `hipDataType`
@emankov emankov mentioned this issue Oct 13, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@daineAMD daineAMD

Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@bragadeesh @emankov @daineAMD