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