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.

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

[Clang] Fix HIP wrapper inclusion of 'algorithm' when using libc++ #67981

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

jhuber6
Copy link
Contributor

@jhuber6 jhuber6 commented Oct 2, 2023

Summary:
The algorithm header included here sometimes caused issues when using
libc++ over libstdc++. This was primarily because of the order they
were included in. This patch just gets rid of this dependency as it was
only used for min and max which are trivial to reimplement.

Fixes: #67938

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics labels Oct 2, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Oct 2, 2023

@llvm/pr-subscribers-backend-x86

@llvm/pr-subscribers-clang

Changes

Summary:
The algorithm header included here sometimes caused issues when using
libc++ over libstdc++. This was primarily because of the order they
were included in. This patch just gets rid of this dependency as it was
only used for min and max which are trivial to reimplement.


Full diff: https://github.com/llvm/llvm-project/pull/67981.diff

1 Files Affected:

  • (modified) clang/lib/Headers/__clang_hip_math.h (+2-5)
diff --git a/clang/lib/Headers/__clang_hip_math.h b/clang/lib/Headers/__clang_hip_math.h
index 58aa55d74769031..11e1e7d032586f8 100644
--- a/clang/lib/Headers/__clang_hip_math.h
+++ b/clang/lib/Headers/__clang_hip_math.h
@@ -14,9 +14,6 @@
 #endif
 
 #if !defined(__HIPCC_RTC__)
-#if defined(__cplusplus)
-#include <algorithm>
-#endif
 #include <limits.h>
 #include <stdint.h>
 #ifdef __OPENMP_AMDGCN__
@@ -1311,11 +1308,11 @@ 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 std::min(__arg1, __arg2);
+  return __arg1 < __arg2 ? __arg1 : __arg2;
 }
 
 __host__ inline static int max(int __arg1, int __arg2) {
-  return std::max(__arg1, __arg2);
+  return __arg1 > __arg2 ? __arg1 : __arg2;
 }
 #endif // !defined(__HIPCC_RTC__) && !defined(__OPENMP_AMDGCN__)
 #endif

Summary:
The `algorithm` header included here sometimes caused issues when using
`libc++` over `libstdc++`. This was primarily because  of the order they
were included in. This patch just gets rid of this dependency as it was
only used for min and max which are trivial to reimplement.

Fixes: llvm#67938
@yxsamliu
Copy link
Collaborator

yxsamliu commented Oct 2, 2023

Did you test it with internal CI? We may need some HIP header changes to avoid regressing existing HIP apps.

@jhuber6
Copy link
Contributor Author

jhuber6 commented Oct 2, 2023

Did you test it with internal CI? We may need some HIP header changes to avoid regressing existing HIP apps.

No, I don't know how to do that. Hopefully people aren't relying on this to be included elsewhere, since its replacement in this single file is pretty striaghtforward.

Copy link
Collaborator

@yxsamliu yxsamliu left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks. It passed internal PSDB

@jhuber6 jhuber6 merged commit 959e69a into llvm:main Oct 2, 2023
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:X86 clang:headers Headers provided by Clang, e.g. for intrinsics clang Clang issues not falling into any other category
Projects
None yet
3 participants