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

Reply via email to