[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
hubert.reinterpretcast accepted this revision. hubert.reinterpretcast added a comment. This revision is now accepted and ready to land. LGTM. Comment at: clang/test/Sema/altivec-generic-overload.c:73 + + __v16sc *gv1_p = convert1(gv1); + __v16uc *gv2_p = convert1(gv2); 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`. https://reviews.llvm.org/D53417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
wuzish marked an inline comment as done.
wuzish added inline comments.
Comment at: clang/test/SemaCXX/vector.cpp:26
float = f1(ll16);
- f1(c16e); // expected-error{{call to 'f1' is ambiguous}}
- f1(ll16e); // expected-error{{call to 'f1' is ambiguous}}
+ f1(c16e);
+ f1(ll16e);
hubert.reinterpretcast wrote:
> Check the return types here like with the two lines above.
I use pointer or reference return value to distinguish the candidate.
https://reviews.llvm.org/D53417
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
wuzish updated this revision to Diff 173981.
wuzish added a comment.
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.
https://reviews.llvm.org/D53417
Files:
clang/lib/Sema/SemaOverload.cpp
clang/test/Sema/altivec-generic-overload.c
clang/test/SemaCXX/vector.cpp
Index: clang/test/SemaCXX/vector.cpp
===
--- clang/test/SemaCXX/vector.cpp
+++ clang/test/SemaCXX/vector.cpp
@@ -17,14 +17,14 @@
f0(ll16e);
}
-int (char16); // expected-note 2{{candidate function}}
-float (longlong16); // expected-note 2{{candidate function}}
+int (char16);
+float (longlong16);
void f1_test(char16 c16, longlong16 ll16, char16_e c16e, longlong16_e ll16e) {
int = f1(c16);
float = f1(ll16);
- f1(c16e); // expected-error{{call to 'f1' is ambiguous}}
- f1(ll16e); // expected-error{{call to 'f1' is ambiguous}}
+ int = f1(c16e);
+ float = 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}} \
Index: clang/test/Sema/altivec-generic-overload.c
===
--- /dev/null
+++ clang/test/Sema/altivec-generic-overload.c
@@ -0,0 +1,100 @@
+// 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 =
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
hubert.reinterpretcast added inline comments.
Comment at: clang/test/CodeGen/altivec-generic-overload.c:74
+ convert1(gv1);
+ // CHECK: call void @_Z8convert1Dv16_a(<16 x i8> %{{[0-9]+}})
+ convert1(gv2);
wuzish wrote:
> hubert.reinterpretcast wrote:
> > 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.
> 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?
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);
}
```
Comment at: clang/test/SemaCXX/vector.cpp:26
float = f1(ll16);
- f1(c16e); // expected-error{{call to 'f1' is ambiguous}}
- f1(ll16e); // expected-error{{call to 'f1' is ambiguous}}
+ f1(c16e);
+ f1(ll16e);
Check the return types here like with the two lines above.
https://reviews.llvm.org/D53417
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
wuzish added inline comments.
Comment at: clang/test/CodeGen/altivec-generic-overload.c:74
+ convert1(gv1);
+ // CHECK: call void @_Z8convert1Dv16_a(<16 x i8> %{{[0-9]+}})
+ convert1(gv2);
hubert.reinterpretcast wrote:
> 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.
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?
https://reviews.llvm.org/D53417
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
hubert.reinterpretcast added inline comments.
Comment at: clang/test/CodeGen/altivec-generic-overload.c:74
+ convert1(gv1);
+ // CHECK: call void @_Z8convert1Dv16_a(<16 x i8> %{{[0-9]+}})
+ convert1(gv2);
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.
https://reviews.llvm.org/D53417
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
wuzish marked 2 inline comments as done. wuzish added a comment. The updated patch now extended the scope and can include the case. https://reviews.llvm.org/D53417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
wuzish updated this revision to Diff 173276.
wuzish added a comment.
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.
https://reviews.llvm.org/D53417
Files:
clang/lib/Sema/SemaOverload.cpp
clang/test/CodeGen/altivec-generic-overload.c
clang/test/Sema/altivec-generic-overload.c
clang/test/SemaCXX/vector.cpp
Index: clang/test/SemaCXX/vector.cpp
===
--- clang/test/SemaCXX/vector.cpp
+++ clang/test/SemaCXX/vector.cpp
@@ -17,14 +17,14 @@
f0(ll16e);
}
-int (char16); // expected-note 2{{candidate function}}
-float (longlong16); // expected-note 2{{candidate function}}
+int (char16);
+float (longlong16);
void f1_test(char16 c16, longlong16 ll16, char16_e c16e, longlong16_e ll16e) {
int = f1(c16);
float = f1(ll16);
- f1(c16e); // expected-error{{call to 'f1' is ambiguous}}
- f1(ll16e); // expected-error{{call to 'f1' is ambiguous}}
+ f1(c16e);
+ 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}} \
Index: clang/test/Sema/altivec-generic-overload.c
===
--- /dev/null
+++ clang/test/Sema/altivec-generic-overload.c
@@ -0,0 +1,101 @@
+// 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)));
+
+void __attribute__((__overloadable__)) convert1(vector signed char);
+void __attribute__((__overloadable__)) convert1(vector unsigned char);
+void __attribute__((__overloadable__)) convert1(vector signed short);
+void __attribute__((__overloadable__)) convert1(vector unsigned short);
+void __attribute__((__overloadable__)) convert1(vector signed int);
+void __attribute__((__overloadable__)) convert1(vector unsigned int);
+void __attribute__((__overloadable__)) convert1(vector signed long long);
+void __attribute__((__overloadable__)) convert1(vector unsigned long long);
+void __attribute__((__overloadable__)) convert1(vector signed __int128);
+void __attribute__((__overloadable__)) convert1(vector unsigned __int128);
+void __attribute__((__overloadable__)) convert1(vector float);
+void __attribute__((__overloadable__)) convert1(vector double);
+void __attribute__((__overloadable__)) convert1(vector bool int);
+void __attribute__((__overloadable__)) convert1(vector pixel short);
+void __attribute__((__overloadable__)) convert2(__v16sc);
+void __attribute__((__overloadable__)) convert2(__v16uc);
+void __attribute__((__overloadable__)) convert2(__v8ss);
+void __attribute__((__overloadable__)) convert2(__v8us);
+void __attribute__((__overloadable__)) convert2(__v4si);
+void __attribute__((__overloadable__)) convert2(__v4ui);
+void __attribute__((__overloadable__)) convert2(__v2sll);
+void __attribute__((__overloadable__)) convert2(__v2ull);
+void __attribute__((__overloadable__)) convert2(__v1slll);
+void __attribute__((__overloadable__)) convert2(__v1ulll);
+void __attribute__((__overloadable__)) convert2(__v4f);
+void __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;
+
+ convert1(gv1);
+ convert1(gv2);
+ convert1(gv3);
+ convert1(gv4);
+ convert1(gv5);
+ convert1(gv6);
+
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
wuzish added a comment. In https://reviews.llvm.org/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. https://reviews.llvm.org/D53417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
hubert.reinterpretcast added a comment. > 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. https://reviews.llvm.org/D53417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
rsmith added a comment. 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. https://reviews.llvm.org/D53417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
hubert.reinterpretcast added inline comments.
Comment at: clang/lib/Sema/SemaOverload.cpp:3913
+for (auto Type : Types) {
+ if (S.Context.getCanonicalType(Type)->getTypeClass() != Type::Vector)
+return false;
wuzish wrote:
> hubert.reinterpretcast wrote:
> > wuzish wrote:
> > > hubert.reinterpretcast wrote:
> > > > wuzish wrote:
> > > > > hubert.reinterpretcast wrote:
> > > > > > hubert.reinterpretcast wrote:
> > > > > > > wuzish wrote:
> > > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > > wuzish wrote:
> > > > > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > > > > wuzish wrote:
> > > > > > > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > > > > > > 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`.
> > > > > > > > > > > > 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).
> > > > > > > > > > > An ExtVectorType is a VectorType, so what sort of
> > > > > > > > > > > failures are you hitting?
> > > > > > > > > > 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 = f1(c16);
> > > > > > > > > > float = 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?
> > > > > > > > > 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)` }.
> > > > > > > > >
> > > > > > > > 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**)?
> > > > > > > I gave a concrete suggested narrowing of the scope that does not
> > > > > > > exclude ExtVectorType or other derived types of VectorType. It
>
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
nemanjai added a comment. @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? Comment at: clang/test/Sema/altivec-generic-overload.c:1 +// RUN: %clang_cc1 %s -triple=powerpc64le-unknown-linux -target-feature +altivec -target-feature +vsx -verify -verify-ignore-unexpected=note -pedantic -fsyntax-only + 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? https://reviews.llvm.org/D53417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
wuzish added a comment. 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. https://reviews.llvm.org/D53417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
wuzish marked an inline comment as done.
wuzish added inline comments.
Comment at: clang/lib/Sema/SemaOverload.cpp:3913
+for (auto Type : Types) {
+ if (S.Context.getCanonicalType(Type)->getTypeClass() != Type::Vector)
+return false;
hubert.reinterpretcast wrote:
> wuzish wrote:
> > hubert.reinterpretcast wrote:
> > > wuzish wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > hubert.reinterpretcast wrote:
> > > > > > wuzish wrote:
> > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > wuzish wrote:
> > > > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > > > wuzish wrote:
> > > > > > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > > > > > 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`.
> > > > > > > > > > > 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).
> > > > > > > > > > An ExtVectorType is a VectorType, so what sort of failures
> > > > > > > > > > are you hitting?
> > > > > > > > > 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 = f1(c16);
> > > > > > > > > float = 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?
> > > > > > > > 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)` }.
> > > > > > > >
> > > > > > > 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**)?
> > > > > > 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] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
wuzish marked an inline comment as done. wuzish added a comment. Gentle ping. Could anyone else have a more review? https://reviews.llvm.org/D53417 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
hubert.reinterpretcast added inline comments.
Comment at: clang/lib/Sema/SemaOverload.cpp:3913
+for (auto Type : Types) {
+ if (S.Context.getCanonicalType(Type)->getTypeClass() != Type::Vector)
+return false;
wuzish wrote:
> hubert.reinterpretcast wrote:
> > wuzish wrote:
> > > hubert.reinterpretcast wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > wuzish wrote:
> > > > > > hubert.reinterpretcast wrote:
> > > > > > > wuzish wrote:
> > > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > > wuzish wrote:
> > > > > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > > > > 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`.
> > > > > > > > > > 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).
> > > > > > > > > An ExtVectorType is a VectorType, so what sort of failures
> > > > > > > > > are you hitting?
> > > > > > > > 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 = f1(c16);
> > > > > > > > float = 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?
> > > > > > > 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)` }.
> > > > > > >
> > > > > > 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**)?
> > > > > 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
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
wuzish added inline comments.
Comment at: clang/lib/Sema/SemaOverload.cpp:3913
+for (auto Type : Types) {
+ if (S.Context.getCanonicalType(Type)->getTypeClass() != Type::Vector)
+return false;
hubert.reinterpretcast wrote:
> wuzish wrote:
> > hubert.reinterpretcast wrote:
> > > hubert.reinterpretcast wrote:
> > > > wuzish wrote:
> > > > > hubert.reinterpretcast wrote:
> > > > > > wuzish wrote:
> > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > wuzish wrote:
> > > > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > > > 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`.
> > > > > > > > > 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).
> > > > > > > > An ExtVectorType is a VectorType, so what sort of failures are
> > > > > > > > you hitting?
> > > > > > > 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 = f1(c16);
> > > > > > > float = 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?
> > > > > > 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)` }.
> > > > > >
> > > > > 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**)?
> > > > 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); }
> >
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
hubert.reinterpretcast added inline comments.
Comment at: clang/lib/Sema/SemaOverload.cpp:3913
+for (auto Type : Types) {
+ if (S.Context.getCanonicalType(Type)->getTypeClass() != Type::Vector)
+return false;
wuzish wrote:
> hubert.reinterpretcast wrote:
> > hubert.reinterpretcast wrote:
> > > wuzish wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > wuzish wrote:
> > > > > > hubert.reinterpretcast wrote:
> > > > > > > wuzish wrote:
> > > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > > 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`.
> > > > > > > > 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).
> > > > > > > An ExtVectorType is a VectorType, so what sort of failures are
> > > > > > > you hitting?
> > > > > > 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 = f1(c16);
> > > > > > float = 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?
> > > > > 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)` }.
> > > > >
> > > > 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**)?
> > > 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__
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
wuzish added inline comments.
Comment at: clang/lib/Sema/SemaOverload.cpp:3913
+for (auto Type : Types) {
+ if (S.Context.getCanonicalType(Type)->getTypeClass() != Type::Vector)
+return false;
hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > wuzish wrote:
> > > hubert.reinterpretcast wrote:
> > > > wuzish wrote:
> > > > > hubert.reinterpretcast wrote:
> > > > > > wuzish wrote:
> > > > > > > hubert.reinterpretcast wrote:
> > > > > > > > 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`.
> > > > > > > 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).
> > > > > > An ExtVectorType is a VectorType, so what sort of failures are you
> > > > > > hitting?
> > > > > 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 = f1(c16);
> > > > > float = 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?
> > > > 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)` }.
> > > >
> > > 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**)?
> > 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.
> Sorry, typo in the above case:
> ```
> typedef unsigned int GccType __attribute__((__ext_vector_type__(4)));
> ```
>
OK, I understand what's your meaning. I just wanted to give you the condition
of what your
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
hubert.reinterpretcast added inline comments.
Comment at: clang/lib/Sema/SemaOverload.cpp:3913
+for (auto Type : Types) {
+ if (S.Context.getCanonicalType(Type)->getTypeClass() != Type::Vector)
+return false;
hubert.reinterpretcast wrote:
> wuzish wrote:
> > hubert.reinterpretcast wrote:
> > > wuzish wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > wuzish wrote:
> > > > > > hubert.reinterpretcast wrote:
> > > > > > > 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`.
> > > > > > 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).
> > > > > An ExtVectorType is a VectorType, so what sort of failures are you
> > > > > hitting?
> > > > 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 = f1(c16);
> > > > float = 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?
> > > 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)` }.
> > >
> > 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**)?
> 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.
Sorry, typo in the above case:
```
typedef unsigned int GccType __attribute__((__ext_vector_type__(4)));
```
https://reviews.llvm.org/D53417
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
hubert.reinterpretcast added inline comments.
Comment at: clang/lib/Sema/SemaOverload.cpp:3913
+for (auto Type : Types) {
+ if (S.Context.getCanonicalType(Type)->getTypeClass() != Type::Vector)
+return false;
wuzish wrote:
> hubert.reinterpretcast wrote:
> > wuzish wrote:
> > > hubert.reinterpretcast wrote:
> > > > wuzish wrote:
> > > > > hubert.reinterpretcast wrote:
> > > > > > 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`.
> > > > > 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).
> > > > An ExtVectorType is a VectorType, so what sort of failures are you
> > > > hitting?
> > > 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 = f1(c16);
> > > float = 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?
> > 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)` }.
> >
> 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**)?
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.
https://reviews.llvm.org/D53417
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
wuzish added inline comments.
Comment at: clang/lib/Sema/SemaOverload.cpp:3913
+for (auto Type : Types) {
+ if (S.Context.getCanonicalType(Type)->getTypeClass() != Type::Vector)
+return false;
hubert.reinterpretcast wrote:
> wuzish wrote:
> > hubert.reinterpretcast wrote:
> > > wuzish wrote:
> > > > hubert.reinterpretcast wrote:
> > > > > 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`.
> > > > 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).
> > > An ExtVectorType is a VectorType, so what sort of failures are you
> > > hitting?
> > 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 = f1(c16);
> > float = 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?
> 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)` }.
>
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**)?
https://reviews.llvm.org/D53417
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
hubert.reinterpretcast added inline comments.
Comment at: clang/lib/Sema/SemaOverload.cpp:3913
+for (auto Type : Types) {
+ if (S.Context.getCanonicalType(Type)->getTypeClass() != Type::Vector)
+return false;
wuzish wrote:
> hubert.reinterpretcast wrote:
> > wuzish wrote:
> > > hubert.reinterpretcast wrote:
> > > > 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`.
> > > 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).
> > An ExtVectorType is a VectorType, so what sort of failures are you hitting?
> 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 = f1(c16);
> float = 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?
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)` }.
https://reviews.llvm.org/D53417
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
wuzish added inline comments.
Comment at: clang/lib/Sema/SemaOverload.cpp:3913
+for (auto Type : Types) {
+ if (S.Context.getCanonicalType(Type)->getTypeClass() != Type::Vector)
+return false;
hubert.reinterpretcast wrote:
> wuzish wrote:
> > hubert.reinterpretcast wrote:
> > > 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`.
> > 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).
> An ExtVectorType is a VectorType, so what sort of failures are you hitting?
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 = f1(c16);
float = 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?
https://reviews.llvm.org/D53417
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
hubert.reinterpretcast added inline comments.
Comment at: clang/lib/Sema/SemaOverload.cpp:3913
+for (auto Type : Types) {
+ if (S.Context.getCanonicalType(Type)->getTypeClass() != Type::Vector)
+return false;
wuzish wrote:
> hubert.reinterpretcast wrote:
> > 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`.
> 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).
An ExtVectorType is a VectorType, so what sort of failures are you hitting?
https://reviews.llvm.org/D53417
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
wuzish added inline comments.
Comment at: clang/lib/Sema/SemaOverload.cpp:3913
+for (auto Type : Types) {
+ if (S.Context.getCanonicalType(Type)->getTypeClass() != Type::Vector)
+return false;
hubert.reinterpretcast wrote:
> 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`.
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).
https://reviews.llvm.org/D53417
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D53417: [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
hubert.reinterpretcast added inline comments.
Comment at: clang/lib/Sema/SemaOverload.cpp:3913
+for (auto Type : Types) {
+ if (S.Context.getCanonicalType(Type)->getTypeClass() != Type::Vector)
+return false;
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`.
https://reviews.llvm.org/D53417
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
