This is an archive of the discontinued LLVM Phabricator instance.

[Clang][Sema]Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error
ClosedPublic

Authored by wuzish on Oct 19 2018, 12:15 AM.

Details

Summary

There are 2 function variations with vector type parameter. When we call them with argument of different vector type we would prefer to choose the variation with implicit argument conversion of compatible vector type instead of incompatible vector type. For example,

typedef float __v4sf __attribute__((__vector_size__(16)));
void f(vector float);
void f(vector signed int);

int main {
   __v4sf a;
   f(a);
}

Here, we'd like to choose f(vector float) but not report an ambiguous call error.

Diff Detail

Repository
rC Clang

Event Timeline

wuzish created this revision.Oct 19 2018, 12:15 AM
wuzish retitled this revision from [Clang][PowerPC] Choose a better candidate as function call if there is a compatible vector conversion instead of ambious call error to [Clang][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambious call error.Oct 19 2018, 12:17 AM
wuzish edited the summary of this revision. (Show Details)
wuzish added a reviewer: steven.zhang.

If anybody knows who are familiar with C/C++ function overload and code related to this issue, please feel free to add them as reviewers and subscribers.

wuzish retitled this revision from [Clang][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambious call error to [Clang][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error.Oct 19 2018, 12:21 AM
clang/lib/Sema/SemaOverload.cpp
3908 ↗(On Diff #170147)

These asserts would be redundant if castAs is used instead of getAs.

3911 ↗(On Diff #170147)

Use castAs here and below.

3930 ↗(On Diff #170147)

s/but/and/;

clang/test/Sema/altivec-generic-overload.c
3 ↗(On Diff #170147)

__v4sc is suspicious. The most consistent naming I have seen is v16i8, v16u8, v8i16, ...
Do we need the double underscores?

hubert.reinterpretcast requested changes to this revision.Oct 19 2018, 7:04 PM
hubert.reinterpretcast added inline comments.
clang/lib/Sema/SemaOverload.cpp
3920 ↗(On Diff #170147)

s/to/over a/;

3941 ↗(On Diff #170147)

This seems to duplicate the bug described here in https://bugs.llvm.org/show_bug.cgi?id=39361.

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); }
This revision now requires changes to proceed.Oct 19 2018, 7:04 PM
wuzish added inline comments.Oct 19 2018, 7:06 PM
clang/test/Sema/altivec-generic-overload.c
3 ↗(On Diff #170147)

Because it's generic gcc vector type instead of altivec type so I choose the naming style.

clang/test/Sema/altivec-generic-overload.c
3 ↗(On Diff #170147)

The naming style from clang/lib/Headers/avxintrin.h would still say that the __v4sc here should be __v16qi.

wuzish marked 8 inline comments as done.Oct 24 2018, 6:43 AM
wuzish added inline comments.
clang/lib/Sema/SemaOverload.cpp
3941 ↗(On Diff #170147)

Thank you for the good case. I will fix it.

But the https://bugs.llvm.org/show_bug.cgi?id=39361 is not the same reason but similar. We can propose another patch to do this. I think the essential reason is that code as blow has not considered ImplicitConversionSequence::Worse so that outside caller goes into path to choose a non-template candidate first.

if (S.getLangOpts().MSVCCompat && SCS1.Second == ICK_Integral_Conversion &&
      SCS2.Second == ICK_Floating_Integral &&
      S.Context.getTypeSize(SCS1.getFromType()) ==
          S.Context.getTypeSize(SCS1.getToType(2)))
    return ImplicitConversionSequence::Better;
clang/test/Sema/altivec-generic-overload.c
3 ↗(On Diff #170147)

On, you mean the num. Yes, that should not be '4'.

wuzish marked 2 inline comments as done.

Updated the diff.

fix some points from comments.

wuzish retitled this revision from [Clang][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error to [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error.Oct 24 2018, 7:08 PM
wuzish edited reviewers, added: rsmith; removed: Douglasgido.
clang/lib/Sema/SemaOverload.cpp
3913 ↗(On Diff #170871)

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.

wuzish added inline comments.Oct 25 2018, 7:19 PM
clang/lib/Sema/SemaOverload.cpp
3913 ↗(On Diff #170871)

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).

clang/lib/Sema/SemaOverload.cpp
3913 ↗(On Diff #170871)

An ExtVectorType is a VectorType, so what sort of failures are you hitting?

wuzish added inline comments.Oct 25 2018, 8:26 PM
clang/lib/Sema/SemaOverload.cpp
3913 ↗(On Diff #170871)

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?

clang/lib/Sema/SemaOverload.cpp
3913 ↗(On Diff #170871)

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) }.

wuzish added inline comments.Oct 25 2018, 10:39 PM
clang/lib/Sema/SemaOverload.cpp
3913 ↗(On Diff #170871)

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

clang/lib/Sema/SemaOverload.cpp
3913 ↗(On Diff #170871)

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.

clang/lib/Sema/SemaOverload.cpp
3913 ↗(On Diff #170871)

Sorry, typo in the above case:

typedef unsigned int GccType __attribute__((__ext_vector_type__(4)));
wuzish added inline comments.Oct 31 2018, 11:09 PM
clang/lib/Sema/SemaOverload.cpp
3913 ↗(On Diff #170871)

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?

clang/lib/Sema/SemaOverload.cpp
3913 ↗(On Diff #170871)

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; }
wuzish added inline comments.Nov 2 2018, 1:14 AM
clang/lib/Sema/SemaOverload.cpp
3913 ↗(On Diff #170871)

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); 
}
clang/lib/Sema/SemaOverload.cpp
3913 ↗(On Diff #170871)

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.

wuzish marked an inline comment as done.Nov 4 2018, 7:11 PM
wuzish added inline comments.
clang/lib/Sema/SemaOverload.cpp
3913 ↗(On Diff #170871)

OK, how about keeping the patch now?

Is it LGTM to you?

wuzish marked an inline comment as done.Nov 4 2018, 7:13 PM

Gentle ping. Could anyone else have a more review?

Gentle ping again since it's a critical patch. Could you please have a more review or give it LGTM if it's LGTM to you. Also welcome new comments.

@hubert.reinterpretcast Have your comments been addressed adequately in the latest version of the patch? Do you have an opinion on adding the test case I proposed?

clang/test/Sema/altivec-generic-overload.c
1 ↗(On Diff #170871)

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?

clang/lib/Sema/SemaOverload.cpp
3913 ↗(On Diff #170871)

I am not okay with this. What is your objection to leaving the case I mentioned ambiguous?

clang/test/Sema/altivec-generic-overload.c
1 ↗(On Diff #170871)

That would be nice for cases like this where it is clear what the behaviour should be.

rsmith added a comment.Nov 8 2018, 7:50 PM

I like the direction here, and I'd like to see this applied generally: a conversion sequence that bitcasts a vector should be ranked worse than one that does not, regardless of the kind of vector in use.

some test points check the error report for ambiguous call because of too many implicit cast choices from ext_vector_type to vector type.

It appears the answer is to update these tests and remove the restriction on the type class.

I like the direction here, and I'd like to see this applied generally: a conversion sequence that bitcasts a vector should be ranked worse than one that does not, regardless of the kind of vector in use.

I also would like to make the direction more aggressive.

Extend the scope to all vector type as one comment suggested.

a conversion sequence that bitcasts a vector should be ranked worse than one that does not, regardless of the kind of vector in use.

Which is also made code clearly and consistently, the effect is to update some tests expected behavior.

wuzish marked 2 inline comments as done.Nov 8 2018, 11:26 PM

The updated patch now extended the scope and can include the case.

clang/test/CodeGen/altivec-generic-overload.c
74 ↗(On Diff #173276)

Checking that the call is to the expected target in terms of overload resolution can be achieved by having a distinct return types for each member of the overload set.

wuzish added inline comments.Nov 13 2018, 4:08 AM
clang/test/CodeGen/altivec-generic-overload.c
74 ↗(On Diff #173276)

What's meaning of having a distinct return types for each member of the overload set?

Could you please give a example to show your expect?

clang/test/CodeGen/altivec-generic-overload.c
74 ↗(On Diff #173276)

This can be done with -fsyntax-only:

typedef signed char __v16sc __attribute__((__vector_size__(16)));
typedef unsigned char __v16uc __attribute__((__vector_size__(16)));

__v16sc *__attribute__((__overloadable__)) convert1(vector signed char);
__v16uc *__attribute__((__overloadable__)) convert1(vector unsigned char);

void test()
{
  __v16sc gv1;
  __v16uc gv2;
  _Generic(convert1(gv1), __v16sc *: (void)0);
  _Generic(convert1(gv2), __v16uc *: (void)0);
}
clang/test/SemaCXX/vector.cpp
26 ↗(On Diff #173276)

Check the return types here like with the two lines above.

Use return type to distinguish which overload candidate is chosen because different candidate has different pointer return type which can not be converted implicitly without reporting error.

wuzish marked an inline comment as done.Nov 13 2018, 8:52 PM
wuzish added inline comments.
clang/test/SemaCXX/vector.cpp
26 ↗(On Diff #173276)

I use pointer or reference return value to distinguish the candidate.

LGTM.

clang/test/Sema/altivec-generic-overload.c
73 ↗(On Diff #173981)

Using assignment instead of _Generic produces only a warning when the types mismatch (at least in certain cases), but that should be okay because we are using -verify.

This revision is now accepted and ready to land.Nov 14 2018, 7:17 PM
wuzish edited the summary of this revision. (Show Details)Nov 15 2018, 6:20 PM
wuzish retitled this revision from [Clang][Sema][PowerPC] Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error to [Clang][Sema]Choose a better candidate in overload function call if there is a compatible vector conversion instead of ambiguous call error.Nov 15 2018, 6:57 PM
This revision was automatically updated to reflect the committed changes.
wuzish marked an inline comment as done.