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