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 &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? > > > > > > > > > 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 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? > > > > 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; } > > > > ``` > > > 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); > > > } > > > ``` > > > > > > > > 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. > OK, how about keeping the patch now? > > Is it LGTM to you? I am not okay with this. What is your objection to leaving the case I mentioned ambiguous? ================ 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 + ---------------- nemanjai wrote: > 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? That would be nice for cases like this where it is clear what the behaviour should be. https://reviews.llvm.org/D53417 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits