[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

Sun Nov 4 19:13:35 PST 2018

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 &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?

https://reviews.llvm.org/D53417