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

rocblas: disable Beta API on Windows for HIP < 5.7 #2405

Merged
merged 8 commits into from
Dec 22, 2023
Merged

rocblas: disable Beta API on Windows for HIP < 5.7 #2405

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4c930f1
rocblas: disable Beta API on Windows for HIP < 5.7
apwojcik Sep 20, 2023
9c8ac3c
fix clang-format issue
junliume Sep 20, 2023
185dfae
Merge branch 'develop' into roblas_beta_api
apwojcik Oct 16, 2023
40a96ec
incorporate review feedback
apwojcik Oct 16, 2023
9a94145
Merge branch 'develop' into roblas_beta_api
apwojcik Oct 30, 2023
15d70ef
Merge branch 'develop' into roblas_beta_api
apwojcik Nov 2, 2023
1d5a79c
incorporate review feedback
apwojcik Nov 13, 2023
d3a4ed2
Merge branch 'develop' into roblas_beta_api
apwojcik Nov 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/gemm_v2.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,14 @@
#endif

#if MIOPEN_USE_ROCBLAS
#if defined(_WIN32) && (HIP_PACKAGE_VERSION_FLAT < 5007000000ULL)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apwojcik IIUC this means the ROCBLA_BETA_FATURES_API would always be enabled on Linux, regardless of the HIP version, which is not what we want ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#if defined(_WIN32) && (HIP_PACKAGE_VERSION_FLAT < 5007000000ULL)
#if defined(_WIN32) || (HIP_PACKAGE_VERSION_FLAT < 5007000000ULL)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@apwojcik Would you agree? Perhaps I am missing something.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was no check before and on Linux Beta Feature API was always on. I made it optional on Windows and available only for HIP SDK 5.7, but not for HIP SDK 5.5 (which is officialy released on Windows and there is no newer available).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change as you suggesting so all using older HIP SDK will compile on Linux, too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JehandadKhan do you want me to change that for Linux, too? By default, Linux installs the latest available ROCm - currently 5.7

#define ROCBLAS_BETA_FEATURES_API 0
#else
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wunused-macros"
#define ROCBLAS_BETA_FEATURES_API 1
#pragma clang diagnostic pop
#endif
#if !defined(_WIN32) && (HIP_PACKAGE_VERSION_FLAT >= 5006000000ULL)
#include <half/half.hpp>
#else
Expand Down Expand Up @@ -114,11 +118,14 @@ static inline rocblas_datatype rocBlasComputeType(const miopen::GemmDescriptor&

auto rocBlasDataType(miopenDataType_t data_type)
{
#if ROCBLAS_BETA_FEATURE_API
junliume marked this conversation as resolved.
Show resolved Hide resolved
junliume marked this conversation as resolved.
Show resolved Hide resolved
if(data_type == miopenFloat8)
return rocblas_datatype::rocblas_datatype_f8_r;
else if(data_type == miopenBFloat8)
return rocblas_datatype::rocblas_datatype_bf8_r;
else if(data_type == miopenHalf)
else
#endif
if(data_type == miopenHalf)
return rocblas_datatype::rocblas_datatype_f16_r;
MIOPEN_THROW(miopenStatusInternalError, "Invalid data type passed");
}
Expand Down
4 changes: 4 additions & 0 deletions src/include/miopen/handle.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,14 @@
#include <unordered_map>

#if MIOPEN_USE_ROCBLAS
#if defined(_WIN32) && (HIP_PACKAGE_VERSION_FLAT < 5007000000ULL)
#define ROCBLAS_BETA_FEATURES_API 0
#else
#pragma clang diagnostic push
#pragma clang diagnostic ignored "-Wunused-macros"
#define ROCBLAS_BETA_FEATURES_API 1
#pragma clang diagnostic pop
#endif
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Side note] @JehandadKhan BTW is seems like handle.hpp does not use any beta API features from rocblas. Shall we simply remove ROCBLAS_BETA_FEATURES_API here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JehandadKhan Oh, no. We have to define ROCBLAS_BETA_FEATURES_API in each header where rocblas.h is included. Because the order of inclusion of headers is not guaranteed, and rocblas.h in included only once, at the first appearance!

Copy link
Contributor

@atamazov atamazov Oct 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JehandadKhan ...and, as gemm_v2.cpp includes handle.hpp, removal of ROCBLAS_BETA_FEATURES_API from handle.hpp would lead to that rocblas.h is included (from handle.hpp) without beta stuff.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JehandadKhan

We have to define ROCBLAS_BETA_FEATURES_API in each header where rocblas.h is included

I've quickly inspected the code and found that rocblas.h is included only in handle.hpp and gemm_v2.cpp, so we are fine.

[end of the side thread]

#include <miopen/manage_ptr.hpp>
#if MIOPEN_ROCBLAS_VERSION_FLAT < 2045000
#include <rocblas.h>
Expand Down