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
