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 &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; } ``` https://reviews.llvm.org/D53417 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits