[OpenCL] Fix overloading ranking rules to work correctly for address space conversions

[Anastasia]

Summary

This bug was pointed out in https://reviews.llvm.org/D55850#inline-496966

[rjmccall]

Is there a reason not to just do T1.getQualifiers() == T2.getQualifiers()?

[Anastasia]

In D56735#1358222, @rjmccall wrote:

Is there a reason not to just do T1.getQualifiers() == T2.getQualifiers()?

I tried this but it causes ObjC test to fail test/SemaObjCXX/arc-overloading.mm.

38 // Simple overloading
39 int &f2(id __strong const *); // expected-note{{candidate function}}
40 float &f2(id __autoreleasing const *); // expected-note{{candidate function}}
41 
42 void test_f2() {
43   id __strong *sip;
44   id __strong const *csip;
45   id __weak *wip;
46   id __autoreleasing *aip;
47   id __unsafe_unretained *uip;
48 
49   // Prefer non-ownership conversions to ownership conversions.
50   int &ir1 = f2(sip);
51   int &ir2 = f2(csip);
52   float &fr1 = f2(aip);
53 
54   f2(uip); // expected-error{{call to 'f2' is ambiguous}}
55 }

Clang fails to resolve the overloads:

error: 'error' diagnostics seen but not expected: 
  File /data/llvm/llvm/tools/clang/test/SemaObjCXX/arc-overloading.mm Line 50: call to 'f2' is ambiguous
  File /data/llvm/llvm/tools/clang/test/SemaObjCXX/arc-overloading.mm Line 52: call to 'f2' is ambiguous
error: 'note' diagnostics seen but not expected: 
  File /data/llvm/llvm/tools/clang/test/SemaObjCXX/arc-overloading.mm Line 39: candidate function
  File /data/llvm/llvm/tools/clang/test/SemaObjCXX/arc-overloading.mm Line 40: candidate function
  File /data/llvm/llvm/tools/clang/test/SemaObjCXX/arc-overloading.mm Line 39: candidate function
  File /data/llvm/llvm/tools/clang/test/SemaObjCXX/arc-overloading.mm Line 40: candidate function

Since CVR quals for both overloads of f2 are the same it didn't continue ranking the other qualifiers before. But now when it continues looking at the other qualifiers it finds that __strong and __autoreleasing are disjoint and exits at the following return statement in CompareQualificationConversions:

} else {
  // Qualifiers are disjoint.
  return ImplicitConversionSequence::Indistinguishable;
}

I feel the logic here doesn't apply to this case because it doesn't take into account that for the passed in function argument there is no ownership qualification conversion for one of the overloads. But I don't have a good idea how to fix this easily other than just keep fast pathing ObjC qual here.

May be if we add a separate qualification conversion for ObjC ownership qualification only we will end up with more conversion steps for one overload and therefore it will be possible to resolve the ambiguity? Do you have any other suggestions or do you think someone else could help?

[rjmccall]

In D56735#1359590, @Anastasia wrote:

In D56735#1358222, @rjmccall wrote:

Is there a reason not to just do T1.getQualifiers() == T2.getQualifiers()?

I tried this but it causes ObjC test to fail test/SemaObjCXX/arc-overloading.mm.

38 // Simple overloading
39 int &f2(id __strong const *); // expected-note{{candidate function}}
40 float &f2(id __autoreleasing const *); // expected-note{{candidate function}}
41 
42 void test_f2() {
43   id __strong *sip;
44   id __strong const *csip;
45   id __weak *wip;
46   id __autoreleasing *aip;
47   id __unsafe_unretained *uip;
48 
49   // Prefer non-ownership conversions to ownership conversions.
50   int &ir1 = f2(sip);
51   int &ir2 = f2(csip);
52   float &fr1 = f2(aip);
53 
54   f2(uip); // expected-error{{call to 'f2' is ambiguous}}
55 }

Clang fails to resolve the overloads:

error: 'error' diagnostics seen but not expected: 
  File /data/llvm/llvm/tools/clang/test/SemaObjCXX/arc-overloading.mm Line 50: call to 'f2' is ambiguous
  File /data/llvm/llvm/tools/clang/test/SemaObjCXX/arc-overloading.mm Line 52: call to 'f2' is ambiguous
error: 'note' diagnostics seen but not expected: 
  File /data/llvm/llvm/tools/clang/test/SemaObjCXX/arc-overloading.mm Line 39: candidate function
  File /data/llvm/llvm/tools/clang/test/SemaObjCXX/arc-overloading.mm Line 40: candidate function
  File /data/llvm/llvm/tools/clang/test/SemaObjCXX/arc-overloading.mm Line 39: candidate function
  File /data/llvm/llvm/tools/clang/test/SemaObjCXX/arc-overloading.mm Line 40: candidate function

Since CVR quals for both overloads of f2 are the same it didn't continue ranking the other qualifiers before. But now when it continues looking at the other qualifiers it finds that __strong and __autoreleasing are disjoint and exits at the following return statement in CompareQualificationConversions:

} else {
  // Qualifiers are disjoint.
  return ImplicitConversionSequence::Indistinguishable;
}

I feel the logic here doesn't apply to this case because it doesn't take into account that for the passed in function argument there is no ownership qualification conversion for one of the overloads. But I don't have a good idea how to fix this easily other than just keep fast pathing ObjC qual here.

May be if we add a separate qualification conversion for ObjC ownership qualification only we will end up with more conversion steps for one overload and therefore it will be possible to resolve the ambiguity? Do you have any other suggestions or do you think someone else could help?

For now, let's just remove the ownership qualifiers before comparing and leave a comment explaining that considering them here seems to interfere with the ARC overload rule.

[Anastasia]

• Changed the condition to use all qualifiers except for ObjC ownership ones.

[rjmccall]

Thanks, LGTM.

Well, actually, could you improve the test case to verify that the right overload is called? This should be easy because C++ type-checking is bottom-up: just make the functions return something distinguishable, e.g. structs that declare different members.

[Anastasia]

Improved test.

[rjmccall]

Thanks!

lib/Sema/SemaOverload.cpp

while (S.Context.UnwrapSimilarTypes(T1, T2)) {
     // Within each iteration of the loop, we check the qualifiers to
     // determine if this still looks like a qualification
     // conversion. Then, if all is well, we unwrap one more level of
     // pointers or pointers-to-members and do it all again
     // until there are no more pointers or pointers-to-members left
     // to unwrap. This essentially mimics what
     // IsQualificationConversion does, but here we're checking for a
     // strict subset of qualifiers.
-    if (T1.getCVRQualifiers() == T2.getCVRQualifiers())
+    if (T1.getQualifiers().withoutObjCLifetime() ==
+        T2.getQualifiers().withoutObjCLifetime())
       // The qualifiers are the same, so this doesn't tell us anything
       // about how the sequences rank.
+      // ObjC ownership quals are omitted above as they interfere with
+      // the ARC overload rule.
       ;
     else if (T2.isMoreQualifiedThan(T1)) {
       // T1 has fewer qualifiers, so it could be the better sequence.
       if (Result == ImplicitConversionSequence::Worse)
         // Neither has qualifiers that are a subset of the other's
         // qualifiers.
         return ImplicitConversionSequence::Indistinguishable;
 

test/SemaOpenCLCXX/address_space_overloading.cl

+// RUN: %clang_cc1 %s -verify -pedantic -fsyntax-only -cl-std=c++
+
+// expected-no-diagnostics
+
+struct RetGlob {
+  int dummy;
+};
+
+struct RetGen {
+  char dummy;
+};
+
+RetGlob foo(const __global int *);
+RetGen foo(const __generic int *);
+
+void kernel k() {
+  __global int *ArgGlob;
+  __generic int *ArgGen;
+  __local int *ArgLoc;
+  RetGlob TestGlob = foo(ArgGlob);
+  RetGen TestGen = foo(ArgGen);
+  TestGen = foo(ArgLoc);
+}