[Clang][Sema]Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error

[• wuzish]

Summary

There are 2 function variations with vector type parameter. When we call them with argument of different vector type we would prefer to choose the variation with implicit argument conversion of compatible vector type instead of incompatible vector type. For example,

typedef float __v4sf __attribute__((__vector_size__(16)));
void f(vector float);
void f(vector signed int);

int main {
   __v4sf a;
   f(a);
}

Here, we'd like to choose f(vector float) but not report an ambiguous call error.

[• wuzish]

If anybody knows who are familiar with C/C++ function overload and code related to this issue, please feel free to add them as reviewers and subscribers.

[hubert.reinterpretcast]

clang/lib/Sema/SemaOverload.cpp

3908 ↗

(On Diff #170147)

These asserts would be redundant if castAs is used instead of getAs.

3911 ↗

(On Diff #170147)

Use castAs here and below.

3930 ↗

(On Diff #170147)

s/but/and/;

[hubert.reinterpretcast]

clang/test/Sema/altivec-generic-overload.c

3 ↗

(On Diff #170147)

__v4sc is suspicious. The most consistent naming I have seen is v16i8, v16u8, v8i16, ...
Do we need the double underscores?

[hubert.reinterpretcast]

clang/lib/Sema/SemaOverload.cpp

3920 ↗

(On Diff #170147)

s/to/over a/;

3941 ↗

(On Diff #170147)

This seems to duplicate the bug described here in https://bugs.llvm.org/show_bug.cgi?id=39361.

typedef unsigned int GccType __attribute__((__vector_size__(16)));
typedef __vector unsigned int AltiVecType;

typedef float GccOtherType __attribute__((__vector_size__(16)));

char *f(GccOtherType, int);
template <typename T> int f(AltiVecType, T);
template <typename T> int g(AltiVecType, T);
char *g(GccOtherType, int);

bool zip(GccType v) { return f(v, 0) == g(v, 0); }

[• wuzish]

clang/test/Sema/altivec-generic-overload.c

3 ↗

(On Diff #170147)

Because it's generic gcc vector type instead of altivec type so I choose the naming style.

[hubert.reinterpretcast]

clang/test/Sema/altivec-generic-overload.c

3 ↗

(On Diff #170147)

The naming style from clang/lib/Headers/avxintrin.h would still say that the __v4sc here should be __v16qi.

[• wuzish]

clang/lib/Sema/SemaOverload.cpp

3941 ↗

(On Diff #170147)

Thank you for the good case. I will fix it.

But the https://bugs.llvm.org/show_bug.cgi?id=39361 is not the same reason but similar. We can propose another patch to do this. I think the essential reason is that code as blow has not considered ImplicitConversionSequence::Worse so that outside caller goes into path to choose a non-template candidate first.

if (S.getLangOpts().MSVCCompat && SCS1.Second == ICK_Integral_Conversion &&
      SCS2.Second == ICK_Floating_Integral &&
      S.Context.getTypeSize(SCS1.getFromType()) ==
          S.Context.getTypeSize(SCS1.getToType(2)))
    return ImplicitConversionSequence::Better;

clang/test/Sema/altivec-generic-overload.c

3 ↗

(On Diff #170147)

On, you mean the num. Yes, that should not be '4'.

[• wuzish]

Updated the diff.

fix some points from comments.

[hubert.reinterpretcast]

clang/lib/Sema/SemaOverload.cpp

3913 ↗

(On Diff #170871)

Considering that this is a local lambda called in one place, are we expecting cases where the canonical type is not something on which we can call getVectorKind()? If not, then we do not need this if.

[• wuzish]

clang/lib/Sema/SemaOverload.cpp

3913 ↗

(On Diff #170871)

Well, that's for ExtVectorType. I encounter some testcase failure because of that. So I just narrow the condition to only handle Type::Vector.

As the following comment said, so I treat it separately.

/ ExtVectorType - Extended vector type. This type is created using
/ attribute((ext_vector_type(n)), where "n" is the number of elements.
/ Unlike vector_size, ext_vector_type is only allowed on typedef's. This
/ class enables syntactic extensions, like Vector Components for accessing
/ points (as .xyzw), colors (as .rgba), and textures (modeled after OpenGL
/ Shading Language).

[hubert.reinterpretcast]

clang/lib/Sema/SemaOverload.cpp

3913 ↗

(On Diff #170871)

An ExtVectorType is a VectorType, so what sort of failures are you hitting?

[• wuzish]

clang/lib/Sema/SemaOverload.cpp

3913 ↗

(On Diff #170871)

Yes. But the TypeClass of ExtVectorType is ExtVector.

some test points check the error report for ambiguous call because of too many implicit cast choices from ext_vector_type to vector type. Such as blow.

typedef char char16 __attribute__ ((__vector_size__ (16)));
typedef long long longlong16 __attribute__ ((__vector_size__ (16)));
typedef char char16_e __attribute__ ((__ext_vector_type__ (16)));
typedef long long longlong16_e __attribute__ ((__ext_vector_type__ (2)));


void f1_test(char16 c16, longlong16 ll16, char16_e c16e, longlong16_e ll16e) {
  int &ir1 = f1(c16);
  float &fr1 = f1(ll16);
  f1(c16e); // expected-error{{call to 'f1' is ambiguous}}
  f1(ll16e); // expected-error{{call to 'f1' is ambiguous}}
}

If we are gonna widen the condition, we can make a follow-up patch. Or we need include this condition and do this together in this patch?

[hubert.reinterpretcast]

clang/lib/Sema/SemaOverload.cpp

3913 ↗

(On Diff #170871)

The widening that has occurred is in allowing the scope of the change to encompass cases where AltiVec vectors are not sufficiently involved. The change in behaviour makes sense, and perhaps the community may want to pursue it; however, the mandate to make that level of change has not been given.

I do not believe that requiring that the TypeClass be VectorType is the correct narrowing of the scope. Instead, we may want to consider requiring that for each SCS in { SCS1, SCS2 }, there is one AltiVec type and one generic vector type in { SCS.getFromType(), SCS.getToType(2) }.

[• wuzish]

clang/lib/Sema/SemaOverload.cpp

3913 ↗

(On Diff #170871)

The key point is that ExtVector is a kind of typeclass, and its vector kind is generic vector type.

So you think we should encompass ext_vector cases as a part of the scope of this patch? And modify the above cases' expected behavior (remove the expected-error)?

[hubert.reinterpretcast]

clang/lib/Sema/SemaOverload.cpp

3913 ↗

(On Diff #170871)

I gave a concrete suggested narrowing of the scope that does not exclude ExtVectorType or other derived types of VectorType. It also does not change the behaviour of the f1_test case above. We could pursue additional discussion over that case (separable from the current patch) on the mailing list.

I believe my suggestion does do something about this case:

typedef unsigned int GccType __attribute__((__ext_vector_type__(16)));
typedef __vector unsigned int AltiVecType;

typedef float GccOtherType __attribute__((__vector_size__(16)));

void f(GccType);
void f(GccOtherType);

void g(AltiVecType v) { f(v); }

I think that addressing the latter case is within the realm of things that we should consider for this patch. Either way, we should ensure that tests for AltiVec/__ext_vector_type__ conversions are available.

[hubert.reinterpretcast]

clang/lib/Sema/SemaOverload.cpp

3913 ↗

(On Diff #170871)

Sorry, typo in the above case:

typedef unsigned int GccType __attribute__((__ext_vector_type__(4)));

[• wuzish]

clang/lib/Sema/SemaOverload.cpp

3913 ↗

(On Diff #170871)

OK, I understand what's your meaning. I just wanted to give you the condition of what your case showed. Thank you.

In my opinion, I don't think AltiVec/__ext_vector_type__ conversions are available. Because

1. vector_size__/ext_vector_type__ conversion is not available
2. "This class enables syntactic extensions, like Vector Components for accessing......" (as ExtVectorType comments said). So we'd better handle separately.

What do you think of this?

[hubert.reinterpretcast]

clang/lib/Sema/SemaOverload.cpp

3913 ↗

(On Diff #170871)

AltiVec/__ext_vector_type__ conversions are available.

typedef unsigned int GccType __attribute__((__ext_vector_type__(4)));
typedef __vector unsigned int AltiVecType;
AltiVecType f(GccType v) { return v; }

[• wuzish]

clang/lib/Sema/SemaOverload.cpp

3913 ↗

(On Diff #170871)

I am afraid that for each SCS in { SCS1, SCS2 }, there is NOT always one AltiVec type and one generic vector type in { SCS.getFromType(), SCS.getToType(2) }. Like following case you mentioned before.

typedef unsigned int GccType __attribute__((__vector_size__(16)));
typedef __vector unsigned int AltiVecType;

typedef float GccOtherType __attribute__((__vector_size__(16)));

char *f(GccOtherType, int);
template <typename T> int f(AltiVecType, T);
template <typename T> int g(AltiVecType, T);
char *g(GccOtherType, int);

bool zip(GccType v) { return f(v, 0) == g(v, 0); }

So one choice is keeping current patch and do not handle following ext_vector case you gave, the other one is to check there only needs one altivec type in { SCS1.getFromType(), SCS1.getToType(2), SCS2.getFromType(), SCS2.getToType(2)}, which will handle the following case.

typedef unsigned int GccType __attribute__((__ext_vector_type__(4)));
typedef __vector unsigned int AltiVecType;

typedef float GccOtherType __attribute__((__vector_size__(16)));

void f(GccType);
void f(GccOtherType);

void g(AltiVecType v) { f(v); }

Then we need accept this behavior. (It's not in testcases)

typedef char char16 __attribute__ ((__vector_size__ (16)));
typedef char char16_e __attribute__ ((__ext_vector_type__ (16)));

int &f1(char16); 
int &f1(vector float);

void f1_test( char16_e c16e) {
  f1(c16e); // no error, will choose f1(char16); 
}

[hubert.reinterpretcast]

clang/lib/Sema/SemaOverload.cpp

3913 ↗

(On Diff #170871)

I know that it is not always the case that for each SCS in { SCS1, SCS2 }, there is one AltiVec type and one generic vector type in { SCS.getFromType(), SCS.getToType(2) }. What I mean is that when it is indeed not the case, we may consider not applying the extra ordering between SCS1 and SCS2. This does mean that the case I mentioned before remains ambiguous; however, that may be acceptable.

[• wuzish]

clang/lib/Sema/SemaOverload.cpp

3913 ↗

(On Diff #170871)

OK, how about keeping the patch now?

Is it LGTM to you?

[• wuzish]

Gentle ping. Could anyone else have a more review?

[• wuzish]

Gentle ping again since it's a critical patch. Could you please have a more review or give it LGTM if it's LGTM to you. Also welcome new comments.

[nemanjai]

@hubert.reinterpretcast Have your comments been addressed adequately in the latest version of the patch? Do you have an opinion on adding the test case I proposed?

clang/test/Sema/altivec-generic-overload.c

1 ↗

(On Diff #170871)

Do we perhaps want a test case that actually tests which overload was chosen to make sure this doesn't change with any potential future changes to overload resolution?

[hubert.reinterpretcast]

clang/lib/Sema/SemaOverload.cpp

3913 ↗

(On Diff #170871)

I am not okay with this. What is your objection to leaving the case I mentioned ambiguous?

clang/test/Sema/altivec-generic-overload.c

1 ↗

(On Diff #170871)

That would be nice for cases like this where it is clear what the behaviour should be.

[rsmith]

I like the direction here, and I'd like to see this applied generally: a conversion sequence that bitcasts a vector should be ranked worse than one that does not, regardless of the kind of vector in use.

[hubert.reinterpretcast]

some test points check the error report for ambiguous call because of too many implicit cast choices from ext_vector_type to vector type.

It appears the answer is to update these tests and remove the restriction on the type class.

[• wuzish]

In D53417#1292404, @rsmith wrote:

I like the direction here, and I'd like to see this applied generally: a conversion sequence that bitcasts a vector should be ranked worse than one that does not, regardless of the kind of vector in use.

I also would like to make the direction more aggressive.

[• wuzish]

Extend the scope to all vector type as one comment suggested.

a conversion sequence that bitcasts a vector should be ranked worse than one that does not, regardless of the kind of vector in use.

Which is also made code clearly and consistently, the effect is to update some tests expected behavior.

[• wuzish]

The updated patch now extended the scope and can include the case.

[hubert.reinterpretcast]

clang/test/CodeGen/altivec-generic-overload.c

74 ↗

(On Diff #173276)

Checking that the call is to the expected target in terms of overload resolution can be achieved by having a distinct return types for each member of the overload set.

[• wuzish]

clang/test/CodeGen/altivec-generic-overload.c

74 ↗

(On Diff #173276)

What's meaning of having a distinct return types for each member of the overload set?

Could you please give a example to show your expect?

[hubert.reinterpretcast]

clang/test/CodeGen/altivec-generic-overload.c

74 ↗

(On Diff #173276)

This can be done with -fsyntax-only:

typedef signed char __v16sc __attribute__((__vector_size__(16)));
typedef unsigned char __v16uc __attribute__((__vector_size__(16)));

__v16sc *__attribute__((__overloadable__)) convert1(vector signed char);
__v16uc *__attribute__((__overloadable__)) convert1(vector unsigned char);

void test()
{
  __v16sc gv1;
  __v16uc gv2;
  _Generic(convert1(gv1), __v16sc *: (void)0);
  _Generic(convert1(gv2), __v16uc *: (void)0);
}

clang/test/SemaCXX/vector.cpp

26 ↗

(On Diff #173276)

Check the return types here like with the two lines above.

[• wuzish]

Use return type to distinguish which overload candidate is chosen because different candidate has different pointer return type which can not be converted implicitly without reporting error.

[• wuzish]

clang/test/SemaCXX/vector.cpp

26 ↗

(On Diff #173276)

I use pointer or reference return value to distinguish the candidate.

[hubert.reinterpretcast]

LGTM.

clang/test/Sema/altivec-generic-overload.c

73 ↗

(On Diff #173981)

Using assignment instead of _Generic produces only a warning when the types mismatch (at least in certain cases), but that should be okay because we are using -verify.

lib/Sema/SemaOverload.cpp

CompareStandardConversionSequences(Sema &S, SourceLocation Loc,
   // Here, MSVC will call f(int) instead of generating a compile error
   // as clang will do in standard mode.
   if (S.getLangOpts().MSVCCompat && SCS1.Second == ICK_Integral_Conversion &&
       SCS2.Second == ICK_Floating_Integral &&
       S.Context.getTypeSize(SCS1.getFromType()) ==
           S.Context.getTypeSize(SCS1.getToType(2)))
     return ImplicitConversionSequence::Better;
 
+  // Prefer a compatible vector conversion over a lax vector conversion
+  // For example:
+  //
+  // typedef float __v4sf __attribute__((__vector_size__(16)));
+  // void f(vector float);
+  // void f(vector signed int);
+  // int main() {
+  //   __v4sf a;
+  //   f(a);
+  // }
+  // Here, we'd like to choose f(vector float) and not
+  // report an ambiguous call error
+  if (SCS1.Second == ICK_Vector_Conversion &&
+      SCS2.Second == ICK_Vector_Conversion) {
+    bool SCS1IsCompatibleVectorConversion = S.Context.areCompatibleVectorTypes(
+        SCS1.getFromType(), SCS1.getToType(2));
+    bool SCS2IsCompatibleVectorConversion = S.Context.areCompatibleVectorTypes(
+        SCS2.getFromType(), SCS2.getToType(2));
+
+    if (SCS1IsCompatibleVectorConversion != SCS2IsCompatibleVectorConversion)
+      return SCS1IsCompatibleVectorConversion
+                 ? ImplicitConversionSequence::Better
+                 : ImplicitConversionSequence::Worse;
+  }
+
   return ImplicitConversionSequence::Indistinguishable;
 }
 
 /// CompareQualificationConversions - Compares two standard conversion
 /// sequences to determine whether they can be ranked based on their
 /// qualification conversions (C++ 13.3.3.2p3 bullet 3).
 static ImplicitConversionSequence::CompareKind
 CompareQualificationConversions(Sema &S,

test/Sema/altivec-generic-overload.c

+// RUN: %clang_cc1 %s -triple=powerpc64le-unknown-linux -target-feature +altivec -target-feature +vsx -verify -verify-ignore-unexpected=note -pedantic -fsyntax-only
+
+typedef signed char __v16sc __attribute__((__vector_size__(16)));
+typedef unsigned char __v16uc __attribute__((__vector_size__(16)));
+typedef signed short __v8ss __attribute__((__vector_size__(16)));
+typedef unsigned short __v8us __attribute__((__vector_size__(16)));
+typedef signed int __v4si __attribute__((__vector_size__(16)));
+typedef unsigned int __v4ui __attribute__((__vector_size__(16)));
+typedef signed long long __v2sll __attribute__((__vector_size__(16)));
+typedef unsigned long long __v2ull __attribute__((__vector_size__(16)));
+typedef signed __int128 __v1slll __attribute__((__vector_size__(16)));
+typedef unsigned __int128 __v1ulll __attribute__((__vector_size__(16)));
+typedef float __v4f __attribute__((__vector_size__(16)));
+typedef double __v2d __attribute__((__vector_size__(16)));
+
+__v16sc *__attribute__((__overloadable__)) convert1(vector signed char);
+__v16uc *__attribute__((__overloadable__)) convert1(vector unsigned char);
+__v8ss *__attribute__((__overloadable__)) convert1(vector signed short);
+__v8us *__attribute__((__overloadable__)) convert1(vector unsigned short);
+__v4si *__attribute__((__overloadable__)) convert1(vector signed int);
+__v4ui *__attribute__((__overloadable__)) convert1(vector unsigned int);
+__v2sll *__attribute__((__overloadable__)) convert1(vector signed long long);
+__v2ull *__attribute__((__overloadable__)) convert1(vector unsigned long long);
+__v1slll *__attribute__((__overloadable__)) convert1(vector signed __int128);
+__v1ulll *__attribute__((__overloadable__)) convert1(vector unsigned __int128);
+__v4f *__attribute__((__overloadable__)) convert1(vector float);
+__v2d *__attribute__((__overloadable__)) convert1(vector double);
+void __attribute__((__overloadable__)) convert1(vector bool int);
+void __attribute__((__overloadable__)) convert1(vector pixel short);
+
+vector signed char *__attribute__((__overloadable__)) convert2(__v16sc);
+vector unsigned char *__attribute__((__overloadable__)) convert2(__v16uc);
+vector signed short *__attribute__((__overloadable__)) convert2(__v8ss);
+vector unsigned short *__attribute__((__overloadable__)) convert2(__v8us);
+vector signed int *__attribute__((__overloadable__)) convert2(__v4si);
+vector unsigned int *__attribute__((__overloadable__)) convert2(__v4ui);
+vector signed long long *__attribute__((__overloadable__)) convert2(__v2sll);
+vector unsigned long long *__attribute__((__overloadable__)) convert2(__v2ull);
+vector signed __int128 *__attribute__((__overloadable__)) convert2(__v1slll);
+vector unsigned __int128 *__attribute__((__overloadable__)) convert2(__v1ulll);
+vector float *__attribute__((__overloadable__)) convert2(__v4f);
+vector double *__attribute__((__overloadable__)) convert2(__v2d);
+
+void test() {
+  __v16sc gv1;
+  __v16uc gv2;
+  __v8ss gv3;
+  __v8us gv4;
+  __v4si gv5;
+  __v4ui gv6;
+  __v2sll gv7;
+  __v2ull gv8;
+  __v1slll gv9;
+  __v1ulll gv10;
+  __v4f gv11;
+  __v2d gv12;
+
+  vector signed char av1;
+  vector unsigned char av2;
+  vector signed short av3;
+  vector unsigned short av4;
+  vector signed int av5;
+  vector unsigned int av6;
+  vector signed long long av7;
+  vector unsigned long long av8;
+  vector signed __int128 av9;
+  vector unsigned __int128 av10;
+  vector float av11;
+  vector double av12;
+  vector bool int av13;
+  vector pixel short av14;
+
+  __v16sc *gv1_p = convert1(gv1);
+  __v16uc *gv2_p = convert1(gv2);
+  __v8ss *gv3_p = convert1(gv3);
+  __v8us *gv4_p = convert1(gv4);
+  __v4si *gv5_p = convert1(gv5);
+  __v4ui *gv6_p = convert1(gv6);
+  __v2sll *gv7_p = convert1(gv7);
+  __v2ull *gv8_p = convert1(gv8);
+  __v1slll *gv9_p = convert1(gv9);
+  __v1ulll *gv10_p = convert1(gv10);
+  __v4f *gv11_p = convert1(gv11);
+  __v2d *gv12_p = convert1(gv12);
+
+  vector signed char *av1_p = convert2(av1);
+  vector unsigned char *av2_p = convert2(av2);
+  vector signed short *av3_p = convert2(av3);
+  vector unsigned short *av4_p = convert2(av4);
+  vector signed int *av5_p = convert2(av5);
+  vector unsigned int *av6_p = convert2(av6);
+  vector signed long long *av7_p = convert2(av7);
+  vector unsigned long long *av8_p = convert2(av8);
+  vector signed __int128 *av9_p = convert2(av9);
+  vector unsigned __int128 *av10_p = convert2(av10);
+  vector float *av11_p = convert2(av11);
+  vector double *av12_p = convert2(av12);
+  convert2(av13); // expected-error {{call to 'convert2' is ambiguous}}
+  convert2(av14); // expected-error {{call to 'convert2' is ambiguous}}
+}

test/SemaCXX/vector.cpp

 
 void f0_test(char16 c16, longlong16 ll16, char16_e c16e, longlong16_e ll16e) {
   f0(c16);
   f0(ll16);
   f0(c16e);
   f0(ll16e);
 }
 
-int &f1(char16); // expected-note 2{{candidate function}}
-float &f1(longlong16); // expected-note 2{{candidate function}}
+int &f1(char16);
+float &f1(longlong16);
 
 void f1_test(char16 c16, longlong16 ll16, char16_e c16e, longlong16_e ll16e) {
   int &ir1 = f1(c16);
   float &fr1 = f1(ll16);
-  f1(c16e); // expected-error{{call to 'f1' is ambiguous}}
-  f1(ll16e); // expected-error{{call to 'f1' is ambiguous}}
+  int &ir2 = f1(c16e);
+  float &fr2 = f1(ll16e);
 }
 
 void f2(char16_e); // expected-note{{no known conversion from 'longlong16_e' (vector of 2 'long long' values) to 'char16_e' (vector of 16 'char' values) for 1st argument}} \
        // expected-note{{candidate function not viable: no known conversion from 'convertible_to<longlong16_e>' to 'char16_e' (vector of 16 'char' values) for 1st argument}}
 
 void f2_test(char16 c16, longlong16 ll16, char16_e c16e, longlong16_e ll16e) {
   f2(c16);
   f2(ll16);