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

[CUDA][HIP] ignore implicit host/device attr for override #72815

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

yxsamliu
Copy link
Collaborator

When deciding whether a previous function declaration is an overload or override, implicit host/device attrs should not be considered.

This fixes the failure for the following code:

`template
class C {
explicit C() {};
};

template <> C::C() {};
`

The issue was introduced by #72394

sine the template specialization is treated as overload due to implicit host/device attrs are considered for overload/override differentiation.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Nov 20, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Nov 20, 2023

@llvm/pr-subscribers-clang

Author: Yaxun (Sam) Liu (yxsamliu)

Changes

When deciding whether a previous function declaration is an overload or override, implicit host/device attrs should not be considered.

This fixes the failure for the following code:

`template <typename T>
class C {
explicit C() {};
};

template <> C<int>::C() {};
`

The issue was introduced by #72394

sine the template specialization is treated as overload due to implicit host/device attrs are considered for overload/override differentiation.


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

4 Files Affected:

  • (modified) clang/lib/Sema/SemaCUDA.cpp (+2-6)
  • (modified) clang/lib/Sema/SemaOverload.cpp (+4-2)
  • (modified) clang/test/SemaCUDA/implicit-member-target-inherited.cu (+1)
  • (modified) clang/test/SemaCUDA/trivial-ctor-dtor.cu (+16)
diff --git a/clang/lib/Sema/SemaCUDA.cpp b/clang/lib/Sema/SemaCUDA.cpp
index b94f448dabe7517..9dc63ff315a3926 100644
--- a/clang/lib/Sema/SemaCUDA.cpp
+++ b/clang/lib/Sema/SemaCUDA.cpp
@@ -1000,13 +1000,9 @@ void Sema::checkCUDATargetOverload(FunctionDecl *NewFD,
     // should have the same implementation on both sides.
     if (NewTarget != OldTarget &&
         ((NewTarget == CFT_HostDevice &&
-          !(LangOpts.OffloadImplicitHostDeviceTemplates &&
-            isCUDAImplicitHostDeviceFunction(NewFD) &&
-            OldTarget == CFT_Device)) ||
+          !isCUDAImplicitHostDeviceFunction(NewFD)) ||
          (OldTarget == CFT_HostDevice &&
-          !(LangOpts.OffloadImplicitHostDeviceTemplates &&
-            isCUDAImplicitHostDeviceFunction(OldFD) &&
-            NewTarget == CFT_Device)) ||
+          !isCUDAImplicitHostDeviceFunction(OldFD)) ||
          (NewTarget == CFT_Global) || (OldTarget == CFT_Global)) &&
         !IsOverload(NewFD, OldFD, /* UseMemberUsingDeclRules = */ false,
                     /* ConsiderCudaAttrs = */ false)) {
diff --git a/clang/lib/Sema/SemaOverload.cpp b/clang/lib/Sema/SemaOverload.cpp
index 9800d7f1c9cfee9..a33acc184b75274 100644
--- a/clang/lib/Sema/SemaOverload.cpp
+++ b/clang/lib/Sema/SemaOverload.cpp
@@ -1491,8 +1491,10 @@ static bool IsOverloadOrOverrideImpl(Sema &SemaRef, FunctionDecl *New,
     // Don't allow overloading of destructors.  (In theory we could, but it
     // would be a giant change to clang.)
     if (!isa<CXXDestructorDecl>(New)) {
-      Sema::CUDAFunctionTarget NewTarget = SemaRef.IdentifyCUDATarget(New),
-                               OldTarget = SemaRef.IdentifyCUDATarget(Old);
+      Sema::CUDAFunctionTarget NewTarget = SemaRef.IdentifyCUDATarget(
+                                   New, /*IgnoreImplicitHDAttr=*/true),
+                               OldTarget = SemaRef.IdentifyCUDATarget(
+                                   Old, /*IgnoreImplicitHDAttr=*/true);
       if (NewTarget != Sema::CFT_InvalidTarget) {
         assert((OldTarget != Sema::CFT_InvalidTarget) &&
                "Unexpected invalid target.");
diff --git a/clang/test/SemaCUDA/implicit-member-target-inherited.cu b/clang/test/SemaCUDA/implicit-member-target-inherited.cu
index 781199bba6b5a11..ceca0891fc9b03c 100644
--- a/clang/test/SemaCUDA/implicit-member-target-inherited.cu
+++ b/clang/test/SemaCUDA/implicit-member-target-inherited.cu
@@ -39,6 +39,7 @@ struct A2_with_device_ctor {
 };
 // expected-note@-3 {{candidate constructor (the implicit copy constructor) not viable}}
 // expected-note@-4 {{candidate constructor (the implicit move constructor) not viable}}
+// expected-note@-4 {{candidate inherited constructor not viable: call to __device__ function from __host__ function}}
 
 struct B2_with_implicit_default_ctor : A2_with_device_ctor {
   using A2_with_device_ctor::A2_with_device_ctor;
diff --git a/clang/test/SemaCUDA/trivial-ctor-dtor.cu b/clang/test/SemaCUDA/trivial-ctor-dtor.cu
index 1df8adc62bab590..21d698d28492ac3 100644
--- a/clang/test/SemaCUDA/trivial-ctor-dtor.cu
+++ b/clang/test/SemaCUDA/trivial-ctor-dtor.cu
@@ -38,3 +38,19 @@ struct TC : TB<T> {
 };
 
 __device__ TC<int> tc; //expected-error {{dynamic initialization is not supported for __device__, __constant__, __shared__, and __managed__ variables}}
+
+// Check trivial ctor specialization
+template <typename T>
+struct C { //expected-note {{candidate constructor (the implicit copy constructor) not viable}}
+           //expected-note@-1 {{candidate constructor (the implicit move constructor) not viable}}
+    explicit C() {};
+};
+
+template <> C<int>::C() {};
+__device__ C<int> ci_d;
+C<int> ci_h;
+
+// Check non-trivial ctor specialization
+template <> C<float>::C() { static int nontrivial_ctor = 1; } //expected-note {{candidate constructor not viable: call to __host__ function from __device__ function}}
+__device__ C<float> cf_d; //expected-error {{no matching constructor for initialization of 'C<float>'}}
+C<float> cf_h;

Copy link
Member

@Artem-B Artem-B left a comment

Choose a reason for hiding this comment

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

LGTM, with one question.

clang/lib/Sema/SemaCUDA.cpp Outdated Show resolved Hide resolved
When deciding whether a previous function declaration is an
overload or override, implicit host/device attrs should
not be considered.

This fixes the failure for the following code:

`template <typename T>
class C {
    explicit C() {};
};

template <> C<int>::C() {};
`

The issue was introduced by llvm#72394

sine the template specialization is treated as overload
due to implicit host/device attrs are considered for
overload/override differentiation.
@yxsamliu yxsamliu merged commit a1e2c65 into llvm:main Nov 20, 2023
3 checks passed
yxsamliu added a commit that referenced this pull request Nov 23, 2023
…2815)"

This reverts commit a1e2c65.

Revert this patch due to regression. A testcase is:

`template <typename T>
class C {
    explicit C() {};
};

template <> C<int>::C() {};
`
rocm-ci pushed a commit to ROCm/llvm-project that referenced this pull request Dec 15, 2023
Make trivial ctor/dtor implicitly host device functions
so that they can be used to initialize file-scope
device variables to match nvcc behavior.

Fixes: llvm#72261

Fixes: SWDEV-432412

cherry-pick of: llvm#72394

[CUDA][HIP] ignore implicit host/device attr for override

When deciding whether a previous function declaration is an
overload or override, implicit host/device attrs should
not be considered.

This fixes the failure for the following code:

`template <typename T>
class C {
    explicit C() {};
};

template <> C<int>::C() {};
`

The issue was introduced by llvm#72394

sine the template specialization is treated as overload
due to implicit host/device attrs are considered for
overload/override differentiation.

cherry-pick of llvm#72815

Change-Id: Ie896cc0e4d5d82d5af48e08a996a3b392285d9ee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants