Skip to content

[HIP] fix host min/max in header #82956

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

Merged
merged 1 commit into from
Feb 27, 2024
Merged

[HIP] fix host min/max in header #82956

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
72 changes: 66 additions & 6 deletions clang/lib/Headers/__clang_hip_math.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1306,15 +1306,75 @@ float min(float __x, float __y) { return __builtin_fminf(__x, __y); }
__DEVICE__
double min(double __x, double __y) { return __builtin_fmin(__x, __y); }

#if !defined(__HIPCC_RTC__) && !defined(__OPENMP_AMDGCN__)
__host__ inline static int min(int __arg1, int __arg2) {
return __arg1 < __arg2 ? __arg1 : __arg2;
// Define host min/max functions.
#if !defined(__HIPCC_RTC__) && !defined(__OPENMP_AMDGCN__) && \
!defined(__HIP_NO_HOST_MIN_MAX_IN_GLOBAL_NAMESPACE__)

#pragma push_macro("DEFINE_MIN_MAX_FUNCTIONS")
#pragma push_macro("DEFINE_MIN_MAX_FUNCTIONS")
#define DEFINE_MIN_MAX_FUNCTIONS(ret_type, type1, type2) \
inline ret_type min(const type1 __a, const type2 __b) { \
return (__a < __b) ? __a : __b; \
} \
inline ret_type max(const type1 __a, const type2 __b) { \
return (__a > __b) ? __a : __b; \
}

// Define min and max functions for same type comparisons
DEFINE_MIN_MAX_FUNCTIONS(int, int, int)
DEFINE_MIN_MAX_FUNCTIONS(unsigned int, unsigned int, unsigned int)
DEFINE_MIN_MAX_FUNCTIONS(long, long, long)
DEFINE_MIN_MAX_FUNCTIONS(unsigned long, unsigned long, unsigned long)
DEFINE_MIN_MAX_FUNCTIONS(long long, long long, long long)
DEFINE_MIN_MAX_FUNCTIONS(unsigned long long, unsigned long long,
unsigned long long)

// The host min/max functions below accept mixed signed/unsigned integer
// parameters and perform unsigned comparisons, which may produce unexpected
// results if a signed integer was passed unintentionally. To avoid this
// happening silently, these overloaded functions are not defined by default.
// However, for compatibility with CUDA, they will be defined if users define
// __HIP_DEFINE_MIXED_HOST_MIN_MAX__.
#ifdef __HIP_DEFINE_MIXED_HOST_MIN_MAX__
DEFINE_MIN_MAX_FUNCTIONS(unsigned int, int, unsigned int)
DEFINE_MIN_MAX_FUNCTIONS(unsigned int, unsigned int, int)
DEFINE_MIN_MAX_FUNCTIONS(unsigned long, long, unsigned long)
DEFINE_MIN_MAX_FUNCTIONS(unsigned long, unsigned long, long)
DEFINE_MIN_MAX_FUNCTIONS(unsigned long long, long long, unsigned long long)
DEFINE_MIN_MAX_FUNCTIONS(unsigned long long, unsigned long long, long long)
Comment on lines +1339 to +1344
Copy link
Member

Choose a reason for hiding this comment

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

I assume these are needed in order to avoid errors about ambiguous overload resolution when we pass signed/unsigned arguments.

Normally, if we were to use std::min() function, the user would have to explicitly cast arguments or use std::min<unsigned>() to resolve the issue.

Implicitly converting int->unsigned under the hood is probably not a good idea here as we do not know what the user needs/wants and whether it's a WAI or an error. For min/max converting a negative argument into an unsigned would probably be an error. I think we do need to force users to use one of the all-signed or all-unsigned variants here, too, same as with std::min/max.

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 assume these are needed in order to avoid errors about ambiguous overload resolution when we pass signed/unsigned arguments.

Normally, if we were to use std::min() function, the user would have to explicitly cast arguments or use std::min<unsigned>() to resolve the issue.

Implicitly converting int->unsigned under the hood is probably not a good idea here as we do not know what the user needs/wants and whether it's a WAI or an error. For min/max converting a negative argument into an unsigned would probably be an error. I think we do need to force users to use one of the all-signed or all-unsigned variants here, too, same as with std::min/max.

You are right about that std::min/max does not allow mixed signed/unsigned arguments.

Unfortunately, that is how CUDA defines its host min/max functions, e.g. https://godbolt.org/z/Wxxq9dv91

Not following this could cause incompatibility and regressions.

Copy link
Member

Choose a reason for hiding this comment

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

Not everything CUDA does is the right model to follow. This may be one of the cases where we should improve things, if we can, instead of just copying the broken behavior. Not adding problematic things is easier than removing them later, when they are used, intentionally or not.

Considering that HIP currently does not have those functions, it would suggest that there is probably no existing HIP code depending on them. Existing cuda code which may need those functions will need some amount of porting to HIP, anyway, so fixing the source code could be done as part of the porting effort.

We could put those mixed min/max functions under some preprocessor guard, which would keep them disabled by default. If someone desperately needs them, they would have to specify -DPLEASE_ENABLE_BROKEN_MINMAX_ON_MIXED_SIGNED_UNSIGNED_TYPES.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK. will define them only if __HIP_DEFINE_MIXED_HOST_MIN_MAX__ is defined.

#endif // ifdef __HIP_DEFINE_MIXED_HOST_MIN_MAX__

// Floating-point comparisons using built-in functions
inline float min(float const __a, float const __b) {
return __builtin_fminf(__a, __b);
}
inline double min(double const __a, double const __b) {
return __builtin_fmin(__a, __b);
}
inline double min(float const __a, double const __b) {
return __builtin_fmin(__a, __b);
}
inline double min(double const __a, float const __b) {
return __builtin_fmin(__a, __b);
}

__host__ inline static int max(int __arg1, int __arg2) {
return __arg1 > __arg2 ? __arg1 : __arg2;
inline float max(float const __a, float const __b) {
return __builtin_fmaxf(__a, __b);
}
inline double max(double const __a, double const __b) {
return __builtin_fmax(__a, __b);
}
inline double max(float const __a, double const __b) {
return __builtin_fmax(__a, __b);
}
#endif // !defined(__HIPCC_RTC__) && !defined(__OPENMP_AMDGCN__)
inline double max(double const __a, float const __b) {
return __builtin_fmax(__a, __b);
}

#pragma pop_macro("DEFINE_MIN_MAX_FUNCTIONS")

#endif // !defined(__HIPCC_RTC__) && !defined(__OPENMP_AMDGCN__) &&
// !defined(__HIP_NO_HOST_MIN_MAX_IN_GLOBAL_NAMESPACE__)
#endif

#pragma pop_macro("__DEVICE__")
Expand Down