[C++4OpenCL] Fix overloading resolution of addrspace constructors

[olestrohm]

Summary

This fixes the prioritization of address spaces when choosing a constructor,
stopping them from being considered equally good, which made the construction
of types that could be constructed by more than one of the constructors.

It does this by preferring the most specific address space, which is decided by seeing
if one of the address spaces is a superset of the other, and preferring the other.

Fixes: PR50329

[Anastasia]

clang/lib/Sema/SemaOverload.cpp

9870

I think we should remove the OpenCL check since it is not OpenCL specific rule and you are using common helpers indeed!

I also wonder if this should be applied to all member functions not only ctors since isBetterOverloadCandidate should be used for everything?

However it seems that other members are already handled somehow https://godbolt.org/z/MrWKPKed7. Do you know where this handling comes from?

clang/test/CodeGenOpenCLCXX/addrspace-constructors.clcpp

14

Let's add a CHECK to make sure that the overload with the specific address space is used.

[olestrohm]

clang/lib/Sema/SemaOverload.cpp

9870

It's handled in SemaOverload.cpp:10000 in OverloadCandidateSet::BestViableFunction which checks if the given function is viable or not. For member functions the address space they're defined in and the address space of the variable have to match. Therefore only one of them is valid at a time and they never get checked against each other in isBetterOverloadCandidate.

[Anastasia]

clang/lib/Sema/SemaOverload.cpp

9870

I am quite sure that for the example above both __private and __generic overloads are viable. In fact, if you remove the __private overload the __generic one gets picked.

So I think the logic should be checking for compatibility rather than the exact match i.e. using isAddressSpaceSupersetOf although it might not be calling that helper directly but through the other Qualifier logic.

[olestrohm]

clang/lib/Sema/SemaOverload.cpp

9870

Ah, I think I checked it wrong last time. Going through it again it's actually determined by QualType::isMoreQualifiedThan in the end. I'll look into it more.

[olestrohm]

Made the check more general, it's no longer OpenCL specific and no longer restricted to just constructors, since the check only requires them to be methods.

Also added more Sema tests, including using FileCheck to check that the constructors that should be used are actually used.

[Anastasia]

clang/lib/Sema/SemaOverload.cpp

9870

So did you happen to find where the address space ranking for the members is currently happening for this test case https://godbolt.org/z/MrWKPKed7?

I just want to make sure we don't add any redundant checks... so if let's say we already have a similar logic elsewhere it would be better if we attempt to generalize it or move it entirely here if it's doable. In general, we should try to use general qualifier logic if it works instead of specializing for address spaces.

[olestrohm]

clang/lib/Sema/SemaOverload.cpp

9870

It's handled on line 9661 with a call to CompareImplicitConversionSequences, which then goes through a couple of checks and discovers that the return type of one of the functions is more specialized than the other. However this uses the Conversions part of the candidates, which the constructors don't have.

I did attempt to generalize this further than address spaces, but that caused some tests to fail, so I think it makes sense to only check address spaces here.

[Anastasia]

clang/lib/Sema/SemaOverload.cpp

9870

Ah I see. Is it because the object parameter is not added implicitly in the ctors?

If this is only working for ctors I wonder if we should cast to CXXConstructorDecl instead of CXXMethodDecl or/and also add a comment explaining that?

[olestrohm]

I've reverted to using Constructors and prioritizing based on which constructor is the most qualified.

[Anastasia]

clang/lib/Sema/SemaOverload.cpp

9870

ok, let's also add a comment explaining that ctors are handled separately from the other members because they don't have an object parameter created explicitly (if that is the reason).

[olestrohm]

Added a comment explaining what the check is meant for.

Also added a CHECK-NOT: used to properly test that the __generic constructor is not used.

[Anastasia]

LGTM! Thanks! Please, amend the comment as suggested in the final commit

clang/lib/Sema/SemaOverload.cpp

9870

Method -> General member function

clang/lib/Sema/SemaOverload.cpp

bool clang::isBetterOverloadCandidate(
   // If other rules cannot determine which is better, CUDA preference is used
   // to determine which is better.
   if (S.getLangOpts().CUDA && Cand1.Function && Cand2.Function) {
     FunctionDecl *Caller = dyn_cast<FunctionDecl>(S.CurContext);
     return S.IdentifyCUDAPreference(Caller, Cand1.Function) >
            S.IdentifyCUDAPreference(Caller, Cand2.Function);
   }
 
+  // General member function overloading is handled above, so this only handles
+  // constructors with address spaces.
+  // This only handles address spaces since C++ has no other
+  // qualifier that can be used with constructors.
+  const auto *CD1 = dyn_cast_or_null<CXXConstructorDecl>(Cand1.Function);
+  const auto *CD2 = dyn_cast_or_null<CXXConstructorDecl>(Cand2.Function);
+  if (CD1 && CD2) {
+    LangAS AS1 = CD1->getMethodQualifiers().getAddressSpace();
+    LangAS AS2 = CD2->getMethodQualifiers().getAddressSpace();
+    if (AS1 != AS2) {
+      if (Qualifiers::isAddressSpaceSupersetOf(AS2, AS1))
+        return true;
+      if (Qualifiers::isAddressSpaceSupersetOf(AS2, AS1))
+        return false;
+    }
+  }
+
   return false;
 }
 
 /// Determine whether two declarations are "equivalent" for the purposes of
 /// name lookup and overload resolution. This applies when the same internal/no
 /// linkage entity is defined by two modules (probably by textually including
 /// the same header). In such a case, we don't consider the declarations to
 /// declare the same entity, but we also don't want lookups with both

clang/test/CodeGenOpenCLCXX/addrspace-constructors.clcpp

 // RUN: %clang_cc1 %s -emit-llvm -o - -O0 -triple spir-unknown-unknown | FileCheck %s
 
 // CHECK: %struct.X = type { i32 }
 
 // CHECK: @ci ={{.*}} addrspace(2) constant i32 0
 // CHECK: @gi ={{.*}} addrspace(1) global i32 0
 __constant int ci = 0;
 __global int gi = 0;
 
 struct X {
   int x;
 
   // Local variables are handled in local_addrspace_init.clcpp
-  // FIXME: __private and __generic constructors clash for __private variable
-  // X() /*__generic*/ = default;
+  X() /*__generic*/ : x(0) {}
   X() __private : x(0) {}
   X() __global : x(0) {}
   constexpr X() __constant : x(0) {}
   constexpr X(int x) __constant : x(x) {}
 };
 
 // CHECK: @cx1 ={{.*}} addrspace(2) constant %struct.X zeroinitializer
 // CHECK: @cx2 ={{.*}} addrspace(2) constant %struct.X { i32 1 }
 // CHECK: @gx ={{.*}} addrspace(1) global %struct.X zeroinitializer
 __constant X cx1;
 __constant X cx2(1);
 __global X gx;
 
 // CHECK: @_ZZ1kE3cx1 = internal addrspace(2) constant %struct.X zeroinitializer
 // CHECK: @_ZZ1kE3cx2 = internal addrspace(2) constant %struct.X { i32 1 }
 
 kernel void k() {
-  // Check that the constructor for px is executed
+  // Check that the constructor for px calls the __private constructor.
   // CHECK: %px = alloca %struct.X
   // CHECK-NEXT: call spir_func void @_ZN1XC1Ev(%struct.X* {{.*}}%px)
   __private X px;
 
   __constant X cx1;
   __constant X cx2(1);
   // CHECK-NEXT: ret void
 }

clang/test/SemaOpenCLCXX/addrspace-constructors.clcpp

-// RUN: %clang_cc1 %s -pedantic -verify -fsyntax-only
+// RUN: %clang_cc1 %s -pedantic -ast-dump -verify | FileCheck %s
 
 __constant int g1; // expected-error {{variable in constant address space must be initialized}}
 __constant int g2 = 0;
 
 struct X {
   int x;
+//CHECK:  CXXConstructorDecl
+//CHECK-NOT:  used
+//CHECK-SAME: X 'void (){{.*}} __generic'
+  X() /*__generic*/ : x(0) {}
+//CHECK: CXXConstructorDecl {{.*}} used X 'void (){{.*}} __private'
+  X() __private : x(0) {}
+//CHECK: CXXConstructorDecl {{.*}} used X 'void (){{.*}} __global'
+  X() __global : x(0) {}
   constexpr X() __constant : x(0) {}
   constexpr X(int x) __constant : x(x) {}
 };
 
+__global X gx;
+
 //expected-note@+2{{candidate constructor (the implicit copy constructor) not viable: no known conversion from 'int' to 'const __generic Y' for 1st argument}}
 //expected-note@+1{{candidate constructor (the implicit move constructor) not viable: no known conversion from 'int' to '__generic Y' for 1st argument}}
 struct Y {
   int y;
   Y() __generic = default; // expected-note{{candidate constructor not viable: requires 0 arguments, but 1 was provided}}
 };
 
 kernel void k() {
   __constant X cx1;
   __constant X cx2(1);
   __local X lx;
+  __private X x;
 
   __private Y py;
   __constant Y cy1; // expected-error{{variable in constant address space must be initialized}}
   __constant Y cy2(1); // expected-error{{no matching constructor for initialization of '__constant Y'}}
 }
 
 struct Z {
   int z;