Skip to content

[HIP] Fix __clang_hip_cmath.hip for ambiguity #101341

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

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

yxsamliu
Copy link
Collaborator

If there is a type T which can be converted to both float and double etc but itself is not specialized for __numeric_type, and it is called for math functions eg. fma, it will cause ambiguity with test function of __numeric_type.

Since test is not template, this error is not bypassed by SFINAE. This is a design flaw of __numeric_type. This patch fixes clang wrapper header to use SFINAE to avoid such ambiguity.

Fixes: SWDEV-461604

Fixes: #101239

If there is a type T which can be converted to both float and double etc but itself
is not specialized for __numeric_type, and it is called for math functions eg. fma,
it will cause ambiguity with test function of __numeric_type.

Since test is not template, this error is not bypassed by SFINAE. This is a design
flaw of __numeric_type. This patch fixes clang wrapper header to use SFINAE to
avoid such ambiguity.

Fixes: SWDEV-461604

Fixes: llvm#101239
@yxsamliu yxsamliu requested review from Artem-B and jhuber6 July 31, 2024 14:31
@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 Jul 31, 2024
@llvmbot
Copy link
Member

llvmbot commented Jul 31, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-backend-x86

Author: Yaxun (Sam) Liu (yxsamliu)

Changes

If there is a type T which can be converted to both float and double etc but itself is not specialized for __numeric_type, and it is called for math functions eg. fma, it will cause ambiguity with test function of __numeric_type.

Since test is not template, this error is not bypassed by SFINAE. This is a design flaw of __numeric_type. This patch fixes clang wrapper header to use SFINAE to avoid such ambiguity.

Fixes: SWDEV-461604

Fixes: #101239


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

2 Files Affected:

  • (modified) clang/lib/Headers/__clang_hip_cmath.h (+6-1)
  • (modified) clang/test/Headers/__clang_hip_cmath.hip (+19)
diff --git a/clang/lib/Headers/__clang_hip_cmath.h b/clang/lib/Headers/__clang_hip_cmath.h
index b52d6b7816611..7d982ad9af7ee 100644
--- a/clang/lib/Headers/__clang_hip_cmath.h
+++ b/clang/lib/Headers/__clang_hip_cmath.h
@@ -395,7 +395,12 @@ template <class _Tp> struct __numeric_type {
   // No support for long double, use double instead.
   static double __test(long double);
 
-  typedef decltype(__test(declval<_Tp>())) type;
+  template <typename _U>
+  static auto __test_impl(int) -> decltype(__test(declval<_U>()));
+
+  template <typename _U> static void __test_impl(...);
+
+  typedef decltype(__test_impl<_Tp>(0)) type;
   static const bool value = !is_same<type, void>::value;
 };
 
diff --git a/clang/test/Headers/__clang_hip_cmath.hip b/clang/test/Headers/__clang_hip_cmath.hip
index ed1030b820627..0c9ff4cdd7808 100644
--- a/clang/test/Headers/__clang_hip_cmath.hip
+++ b/clang/test/Headers/__clang_hip_cmath.hip
@@ -87,3 +87,22 @@ extern "C" __device__ float test_sin_f32(float x) {
 extern "C" __device__ float test_cos_f32(float x) {
   return cos(x);
 }
+
+// Check user defined type which can be converted to float and double but not
+// specializes __numeric_type will not cause ambiguity diagnostics.
+struct user_bfloat16 {
+  __host__ __device__ user_bfloat16(float);
+  operator float();
+  operator double();
+};
+
+namespace user_namespace {
+  __device__ user_bfloat16 fma(const user_bfloat16 a, const user_bfloat16 b, const user_bfloat16 c) {
+    return a;
+  }
+
+  __global__ void test_fma() {
+    user_bfloat16 a = 1.0f, b = 2.0f;
+    fma(a, b, b);
+  }
+}

@yxsamliu yxsamliu merged commit db1d3b2 into llvm:main Aug 2, 2024
11 checks passed
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Sep 24, 2024
If there is a type T which can be converted to both float and double etc
but itself is not specialized for __numeric_type, and it is called for
math functions eg. fma, it will cause ambiguity with test function of
__numeric_type.

Since test is not template, this error is not bypassed by SFINAE. This
is a design flaw of __numeric_type. This patch fixes clang wrapper
header to use SFINAE to avoid such ambiguity.

Fixes: SWDEV-461604

Fixes: llvm#101239
Change-Id: I6cd22046aaf3c18871d95244a07ecddad23449a7
searlmc1 pushed a commit to ROCm/llvm-project that referenced this pull request Nov 22, 2024
If there is a type T which can be converted to both float and double etc
but itself is not specialized for __numeric_type, and it is called for
math functions eg. fma, it will cause ambiguity with test function of
__numeric_type.

Since test is not template, this error is not bypassed by SFINAE. This
is a design flaw of __numeric_type. This patch fixes clang wrapper
header to use SFINAE to avoid such ambiguity.

Fixes: SWDEV-461604

Fixes: llvm#101239
Change-Id: I285adbeb7bf8fe57d084625b98fa6fd49092d641
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
Development

Successfully merging this pull request may close these issues.

[HIP] calling math function with user defined type causes ambiguity
3 participants