[PATCH] D27334: [OpenCL] Ambiguous function call.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In https://reviews.llvm.org/D27334#751921, @echuraev wrote:

> So, I think that we have to do some decision about this patch. @Anastasia, 
> What do you think about it? Please see my comment above. What should we do 
> with this patch?


I am still not convinced adding the diagnostics in this shape is a good way to 
go


https://reviews.llvm.org/D27334



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27334: [OpenCL] Ambiguous function call.

[Egor Churaev via Phabricator via cfe-commits]

echuraev added a comment.

So, I think that we have to do some decision about this patch. @Anastasia, What 
do you think about it? Please see my commentary above. What should we do with 
this patch?


https://reviews.llvm.org/D27334



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27334: [OpenCL] Ambiguous function call.

[Egor Churaev via Phabricator via cfe-commits]

echuraev added a comment.

In https://reviews.llvm.org/D27334#700521, @Anastasia wrote:

> I don't actually. But remembering the follow up discussion:
>  http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20161205/178846.html
>  and since we have to deviate from the standard C/C++ implementation anyways 
> I am wondering whether modifying overloading resolution is a better way to go?


Yes, I remember this discussion. But we saw this problem in the real OpenCL 
code and this warning is able to help developer to understand where his code 
could be ambiguous. Actually, I don't know the modifying overloading resolution 
is a better way to go or not. Do you have any suggestions how could we notify 
developer about this potential problem in better way?


https://reviews.llvm.org/D27334



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27334: [OpenCL] Ambiguous function call.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

I don't actually. But remembering the follow up discussion:
http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20161205/178846.html
and since we have to deviate from the standard C/C++ implementation anyways I 
am wondering whether modifying overloading resolution is a better way to go?


https://reviews.llvm.org/D27334



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27334: [OpenCL] Ambiguous function call.

[Egor Churaev via Phabricator via cfe-commits]

echuraev added a comment.

In https://reviews.llvm.org/D27334#614826, @Anastasia wrote:

> In https://reviews.llvm.org/D27334#614389, @bader wrote:
>
> > In https://reviews.llvm.org/D27334#613504, @Anastasia wrote:
> >
> > > In https://reviews.llvm.org/D27334#612858, @bader wrote:
> > >
> > > > In https://reviews.llvm.org/D27334#611703, @Anastasia wrote:
> > > >
> > > > > This change seems to modify normal C behavior again. Is there any 
> > > > > strong motivation for doing this and if yes could it be done 
> > > > > generically with C?
> > > >
> > > >
> > > > Motivation:
> > > >
> > > >   // Non-portable OpenCL 1.2 code 
> > > >   __kernel void foo(global float* out) {
> > > > out[get_global_id(0)] = sin(get_global_id(0));
> > > >   }
> > > >
> > > >
> > > > This program compiles fine on OpenCL platform w/o doubles support and 
> > > > fails otherwise.
> > > >  If OpenCL driver supports doubles it provides at least two versions of 
> > > > 'sin' built-in math function and compiler will not be able to choose 
> > > > the right one for 'size_t' argument.
> > > >  The goal of this warning is to let OpenCL developer know about 
> > > > potential issues with code portability.
> > >
> > >
> > > I would argue this improves the portability much as it can also be 
> > > misleading in some situations (because it refers to a potentially 
> > > hypothetical problem). For example there can be builtin functions that 
> > > only have a float parameter (without a double version of it). This is for 
> > > example the case with read_image functions that take a float coordinate 
> > > value between 0 and 1. Unfortunately this warning won't be triggered on 
> > > read_image functions because there is an overload candidate with an int 
> > > type of the same parameter too. But we can't exclude this situations to 
> > > appear in the future or from some vendor extensions or even custom OpenCL 
> > > code.
> >
> >
> > As much as any other warning it's not always means that there is an error 
> > in the code. It just means that developer should inspect the construction 
> > triggering a warning.
> >  Passing integer value to a function with floating point parameters is not 
> > always an error, but some times it might be so.
> >  Do you suggest dropping the diagnostics at all or changing the diagnostics 
> > message?
>
>
> I agree warnings don't always signal a definite issue (even thought it's good 
> to make them as precise as we can). We could try to reword the diagnostic 
> message. However, the biggest issue I have here is that the message can be 
> given in the situations that are unrelated to the problem (i.e. the overload 
> candidates that don't have anything to do with the parameter being diagnosed 
> or don't overload with the double precision). Therefore, it feels like the 
> diagnostic can be confusing in some cases even though they are not very 
> probable ones.


@Anastasia, do you have any suggestions how is it better to reword the 
diagnostic message? Yes, this message can be given in some situations that are 
unrelated to the problem but in this case it will be a notification for 
developer that this function call can be potential ambiguous.




Comment at: lib/Sema/SemaChecking.cpp:2479
+// integer values.
+if (FDecl->hasAttr()) {
+  for (const auto* Arg : Args) {

Anastasia wrote:
> How does it check that this is a built-in function?
In OpenCL we can overload only built-in functions. So, I think that we can 
recognize that the function is built-in by checking that the language is OpenCL 
and the function has Overloadable attribute.


https://reviews.llvm.org/D27334



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: [PATCH] D27334: [OpenCL] Ambiguous function call.

[Anastasia Stulova via cfe-commits]

I think we can’t avoid deviating from C/C++ implementation completely. But we 
should certainly try to avoid it in unnecessary situations as much as we can.

In this case it seems like the spec enforces the rules for resolving the 
ambiguity that seems logical to me.

Is this possibly a better way to deviate OpenCL implementation than giving a 
hypothetical warning and an error in the case the ambiguity wasn’t resolved?

Cheers,
Anastasia

From: Bader, Alexey [mailto:alexey.ba...@intel.com]
Sent: 06 December 2016 12:17
To: Anastasia Stulova; Richard Smith; Bruno Cardoso Lopes via Phabricator; 
reviews+d27334+public+f2c5a66032c4c...@reviews.llvm.org
Cc: egor.chur...@gmail.com; yaxun@amd.com; cfe-commits@lists.llvm.org; nd
Subject: RE: [PATCH] D27334: [OpenCL] Ambiguous function call.

Actually OpenCL specification has the following text in “Built-in Functions” 
chapter (OpenCL C 2.0 rev. 33):

“ User defined OpenCL C functions, behave per C standard rules for functions 
(C99, TC2, Section
6.9.1). On entry to the function, the size of each variably modified parameter 
is evaluated and
the value of each argument expression is converted to the type of the 
corresponding parameter as
per usual arithmetic conversion rules described in section 6.2.6. Built-in 
functions described in
this section behave similarly, except that in order to avoid ambiguity between 
multiple forms of
the same built-in function, implicit scalar widening shall not occur. Note that 
some built-in
functions described in this section do have forms that operate on mixed scalar 
and vector types,
however.”

If I understand it correctly this text can help to resolve the overloading, 
since “usual arithmetic conversion rules” from section 6.2.6 rank floating 
point and integer data types including vector flavors. On the other hand I 
don’t think it’s good idea to adopt this rule, since it doesn’t follow C++ 
overloading rules and definitely will deviate OpenCL behavior from C or C++.

Thanks,
Alexey

From: Anastasia Stulova [mailto:anastasia.stul...@arm.com]
Sent: Monday, December 5, 2016 9:21 PM
To: Richard Smith <rich...@metafoo.co.uk<mailto:rich...@metafoo.co.uk>>; Bruno 
Cardoso Lopes via Phabricator 
<revi...@reviews.llvm.org<mailto:revi...@reviews.llvm.org>>; 
reviews+d27334+public+f2c5a66032c4c...@reviews.llvm.org<mailto:reviews+d27334+public+f2c5a66032c4c...@reviews.llvm.org>
Cc: Bader, Alexey <alexey.ba...@intel.com<mailto:alexey.ba...@intel.com>>; 
egor.chur...@gmail.com<mailto:egor.chur...@gmail.com>; 
yaxun@amd.com<mailto:yaxun@amd.com>; 
cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>; nd 
<n...@arm.com<mailto:n...@arm.com>>
Subject: RE: [PATCH] D27334: [OpenCL] Ambiguous function call.

> Perhaps that is the problem (that there are two modes that do different 
> things)? Could we make the double overload be present but unselectable to 
> diagnose this problem in that mode too?

If we could resolve the overload candidate to prefer ‘int -> float’ than 
‘int->double’, it would work best.

From: meta...@gmail.com<mailto:meta...@gmail.com> [mailto:meta...@gmail.com] On 
Behalf Of Richard Smith
Sent: 05 December 2016 17:53
To: Bruno Cardoso Lopes via Phabricator; 
reviews+d27334+public+f2c5a66032c4c...@reviews.llvm.org<mailto:reviews+d27334+public+f2c5a66032c4c...@reviews.llvm.org>
Cc: alexey.ba...@intel.com<mailto:alexey.ba...@intel.com>; 
egor.chur...@gmail.com<mailto:egor.chur...@gmail.com>; 
yaxun@amd.com<mailto:yaxun....@amd.com>; Anastasia Stulova; 
cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>
Subject: Re: [PATCH] D27334: [OpenCL] Ambiguous function call.

On 5 Dec 2016 9:42 am, "Anastasia Stulova via Phabricator via cfe-commits" 
<cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>> wrote:
Anastasia added a comment.

In https://reviews.llvm.org/D27334#612858, @bader wrote:

> In https://reviews.llvm.org/D27334#611703, @Anastasia wrote:
>
> > This change seems to modify normal C behavior again. Is there any strong 
> > motivation for doing this and if yes could it be done generically with C?
>
>
> Motivation:
>
>   // Non-portable OpenCL 1.2 code
>   __kernel void foo(global float* out) {
> out[get_global_id(0)] = sin(get_global_id(0));
>   }
>
>
> This program compiles fine on OpenCL platform w/o doubles support

Perhaps that is the problem (that there are two modes that do different 
things)? Could we make the double overload be present but unselectable to 
diagnose this problem in that mode too?

and fails otherwise.
>  If OpenCL driver supports doubles it provides at least two versions of 'sin' 
> built-in math function and compiler will not be able to choose the right one 
> for 'size_t' argument.

Do you have a real example? If someone passes an inte

[PATCH] D27334: [OpenCL] Ambiguous function call.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In https://reviews.llvm.org/D27334#614389, @bader wrote:

> In https://reviews.llvm.org/D27334#613504, @Anastasia wrote:
>
> > In https://reviews.llvm.org/D27334#612858, @bader wrote:
> >
> > > In https://reviews.llvm.org/D27334#611703, @Anastasia wrote:
> > >
> > > > This change seems to modify normal C behavior again. Is there any 
> > > > strong motivation for doing this and if yes could it be done 
> > > > generically with C?
> > >
> > >
> > > Motivation:
> > >
> > >   // Non-portable OpenCL 1.2 code 
> > >   __kernel void foo(global float* out) {
> > > out[get_global_id(0)] = sin(get_global_id(0));
> > >   }
> > >
> > >
> > > This program compiles fine on OpenCL platform w/o doubles support and 
> > > fails otherwise.
> > >  If OpenCL driver supports doubles it provides at least two versions of 
> > > 'sin' built-in math function and compiler will not be able to choose the 
> > > right one for 'size_t' argument.
> > >  The goal of this warning is to let OpenCL developer know about potential 
> > > issues with code portability.
> >
> >
> > I would argue this improves the portability much as it can also be 
> > misleading in some situations (because it refers to a potentially 
> > hypothetical problem). For example there can be builtin functions that only 
> > have a float parameter (without a double version of it). This is for 
> > example the case with read_image functions that take a float coordinate 
> > value between 0 and 1. Unfortunately this warning won't be triggered on 
> > read_image functions because there is an overload candidate with an int 
> > type of the same parameter too. But we can't exclude this situations to 
> > appear in the future or from some vendor extensions or even custom OpenCL 
> > code.
>
>
> As much as any other warning it's not always means that there is an error in 
> the code. It just means that developer should inspect the construction 
> triggering a warning.
>  Passing integer value to a function with floating point parameters is not 
> always an error, but some times it might be so.
>  Do you suggest dropping the diagnostics at all or changing the diagnostics 
> message?


I agree warnings don't always signal a definite issue (even thought it's good 
to make them as precise as we can). We could try to reword the diagnostic 
message. However, the biggest issue I have here is that the message can be 
given in the situations that are unrelated to the problem (i.e. the overload 
candidates that don't have anything to do with the parameter being diagnosed or 
don't overload with the double precision). Therefore, it feels like the 
diagnostic can be confusing in some cases even though they are not very 
probable ones.


https://reviews.llvm.org/D27334



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: [PATCH] D27334: [OpenCL] Ambiguous function call.

[Bader, Alexey via cfe-commits]

Actually OpenCL specification has the following text in “Built-in Functions” 
chapter (OpenCL C 2.0 rev. 33):

“ User defined OpenCL C functions, behave per C standard rules for functions 
(C99, TC2, Section
6.9.1). On entry to the function, the size of each variably modified parameter 
is evaluated and
the value of each argument expression is converted to the type of the 
corresponding parameter as
per usual arithmetic conversion rules described in section 6.2.6. Built-in 
functions described in
this section behave similarly, except that in order to avoid ambiguity between 
multiple forms of
the same built-in function, implicit scalar widening shall not occur. Note that 
some built-in
functions described in this section do have forms that operate on mixed scalar 
and vector types,
however.”

If I understand it correctly this text can help to resolve the overloading, 
since “usual arithmetic conversion rules” from section 6.2.6 rank floating 
point and integer data types including vector flavors. On the other hand I 
don’t think it’s good idea to adopt this rule, since it doesn’t follow C++ 
overloading rules and definitely will deviate OpenCL behavior from C or C++.

Thanks,
Alexey

From: Anastasia Stulova [mailto:anastasia.stul...@arm.com]
Sent: Monday, December 5, 2016 9:21 PM
To: Richard Smith <rich...@metafoo.co.uk>; Bruno Cardoso Lopes via Phabricator 
<revi...@reviews.llvm.org>; 
reviews+d27334+public+f2c5a66032c4c...@reviews.llvm.org
Cc: Bader, Alexey <alexey.ba...@intel.com>; egor.chur...@gmail.com; 
yaxun@amd.com; cfe-commits@lists.llvm.org; nd <n...@arm.com>
Subject: RE: [PATCH] D27334: [OpenCL] Ambiguous function call.

> Perhaps that is the problem (that there are two modes that do different 
> things)? Could we make the double overload be present but unselectable to 
> diagnose this problem in that mode too?

If we could resolve the overload candidate to prefer ‘int -> float’ than 
‘int->double’, it would work best.

From: meta...@gmail.com<mailto:meta...@gmail.com> [mailto:meta...@gmail.com] On 
Behalf Of Richard Smith
Sent: 05 December 2016 17:53
To: Bruno Cardoso Lopes via Phabricator; 
reviews+d27334+public+f2c5a66032c4c...@reviews.llvm.org<mailto:reviews+d27334+public+f2c5a66032c4c...@reviews.llvm.org>
Cc: alexey.ba...@intel.com<mailto:alexey.ba...@intel.com>; 
egor.chur...@gmail.com<mailto:egor.chur...@gmail.com>; 
yaxun@amd.com<mailto:yaxun@amd.com>; Anastasia Stulova; 
cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>
Subject: Re: [PATCH] D27334: [OpenCL] Ambiguous function call.

On 5 Dec 2016 9:42 am, "Anastasia Stulova via Phabricator via cfe-commits" 
<cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>> wrote:
Anastasia added a comment.

In https://reviews.llvm.org/D27334#612858, @bader wrote:

> In https://reviews.llvm.org/D27334#611703, @Anastasia wrote:
>
> > This change seems to modify normal C behavior again. Is there any strong 
> > motivation for doing this and if yes could it be done generically with C?
>
>
> Motivation:
>
>   // Non-portable OpenCL 1.2 code
>   __kernel void foo(global float* out) {
> out[get_global_id(0)] = sin(get_global_id(0));
>   }
>
>
> This program compiles fine on OpenCL platform w/o doubles support

Perhaps that is the problem (that there are two modes that do different 
things)? Could we make the double overload be present but unselectable to 
diagnose this problem in that mode too?

and fails otherwise.
>  If OpenCL driver supports doubles it provides at least two versions of 'sin' 
> built-in math function and compiler will not be able to choose the right one 
> for 'size_t' argument.

Do you have a real example? If someone passes an integer to 'sin', I think 
there's a better warning we can give them :)

>  The goal of this warning is to let OpenCL developer know about potential 
> issues with code portability.
I would argue this improves the portability much as it can also be misleading 
in some situations (because it refers to a potentially hypothetical problem). 
For example there can be builtin functions that only have a float parameter 
(without a double version of it). This is for example the case with read_image 
functions that take a float coordinate value between 0 and 1. Unfortunately 
this warning won't be triggered on read_image functions because there is an 
overload candidate with an int type of the same parameter too. But we can't 
exclude this situations to appear in the future or from some vendor extensions 
or even custom OpenCL code.


https://reviews.llvm.org/D27334



___
cfe-commits mailing list
cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits


-

RE: [PATCH] D27334: [OpenCL] Ambiguous function call.

[Bader, Alexey via cfe-commits]

On 5 Dec 2016 9:42 am, "Anastasia Stulova via Phabricator via cfe-commits" 
> wrote:
Anastasia added a comment.

In https://reviews.llvm.org/D27334#612858, @bader wrote:

> In https://reviews.llvm.org/D27334#611703, @Anastasia wrote:
>
> > This change seems to modify normal C behavior again. Is there any strong 
> > motivation for doing this and if yes could it be done generically with C?
>
>
> Motivation:
>
>   // Non-portable OpenCL 1.2 code
>   __kernel void foo(global float* out) {
> out[get_global_id(0)] = sin(get_global_id(0));
>   }
>
>
> This program compiles fine on OpenCL platform w/o doubles support

Perhaps that is the problem (that there are two modes that do different 
things)? Could we make the double overload be present but unselectable to 
diagnose this problem in that mode too?

and fails otherwise.
>  If OpenCL driver supports doubles it provides at least two versions of 'sin' 
> built-in math function and compiler will not be able to choose the right one 
> for 'size_t' argument.

Do you have a real example? If someone passes an integer to 'sin', I think 
there's a better warning we can give them :)
[AB] As far as I remember the code that I got used some OpenCL math built-in 
function and passed some integer constant like pow(x, 2).
To fix this case is developer can pass ‘float’ literal ‘2.f’ or use ‘pown’ 
built-in math function which accepts integers as second parameter.

>  The goal of this warning is to let OpenCL developer know about potential 
> issues with code portability.

I would argue this improves the portability much as it can also be misleading 
in some situations (because it refers to a potentially hypothetical problem). 
For example there can be builtin functions that only have a float parameter 
(without a double version of it). This is for example the case with read_image 
functions that take a float coordinate value between 0 and 1. Unfortunately 
this warning won't be triggered on read_image functions because there is an 
overload candidate with an int type of the same parameter too. But we can't 
exclude this situations to appear in the future or from some vendor extensions 
or even custom OpenCL code.


https://reviews.llvm.org/D27334



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits



Joint Stock Company Intel A/O
Registered legal address: Krylatsky Hills Business Park,
17 Krylatskaya Str., Bldg 4, Moscow 121614,
Russian Federation

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27334: [OpenCL] Ambiguous function call.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In https://reviews.llvm.org/D27334#613504, @Anastasia wrote:

> In https://reviews.llvm.org/D27334#612858, @bader wrote:
>
> > In https://reviews.llvm.org/D27334#611703, @Anastasia wrote:
> >
> > > This change seems to modify normal C behavior again. Is there any strong 
> > > motivation for doing this and if yes could it be done generically with C?
> >
> >
> > Motivation:
> >
> >   // Non-portable OpenCL 1.2 code 
> >   __kernel void foo(global float* out) {
> > out[get_global_id(0)] = sin(get_global_id(0));
> >   }
> >
> >
> > This program compiles fine on OpenCL platform w/o doubles support and fails 
> > otherwise.
> >  If OpenCL driver supports doubles it provides at least two versions of 
> > 'sin' built-in math function and compiler will not be able to choose the 
> > right one for 'size_t' argument.
> >  The goal of this warning is to let OpenCL developer know about potential 
> > issues with code portability.
>
>
> I would argue this improves the portability much as it can also be misleading 
> in some situations (because it refers to a potentially hypothetical problem). 
> For example there can be builtin functions that only have a float parameter 
> (without a double version of it). This is for example the case with 
> read_image functions that take a float coordinate value between 0 and 1. 
> Unfortunately this warning won't be triggered on read_image functions because 
> there is an overload candidate with an int type of the same parameter too. 
> But we can't exclude this situations to appear in the future or from some 
> vendor extensions or even custom OpenCL code.


As much as any other warning it's not always means that there is an error in 
the code. It just means that developer should inspect the construction 
triggering a warning.
Passing integer value to a function with floating point parameters is not 
always an error, but some times it might be so.
Do you suggest dropping the diagnostics at all or changing the diagnostics 
message?


https://reviews.llvm.org/D27334



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

RE: [PATCH] D27334: [OpenCL] Ambiguous function call.

[Anastasia Stulova via cfe-commits]

> Perhaps that is the problem (that there are two modes that do different 
> things)? Could we make the double overload be present but unselectable to 
> diagnose this problem in that mode too?

If we could resolve the overload candidate to prefer ‘int -> float’ than 
‘int->double’, it would work best.

From: meta...@gmail.com [mailto:meta...@gmail.com] On Behalf Of Richard Smith
Sent: 05 December 2016 17:53
To: Bruno Cardoso Lopes via Phabricator; 
reviews+d27334+public+f2c5a66032c4c...@reviews.llvm.org
Cc: alexey.ba...@intel.com; egor.chur...@gmail.com; yaxun@amd.com; 
Anastasia Stulova; cfe-commits@lists.llvm.org
Subject: Re: [PATCH] D27334: [OpenCL] Ambiguous function call.

On 5 Dec 2016 9:42 am, "Anastasia Stulova via Phabricator via cfe-commits" 
<cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>> wrote:
Anastasia added a comment.

In https://reviews.llvm.org/D27334#612858, @bader wrote:

> In https://reviews.llvm.org/D27334#611703, @Anastasia wrote:
>
> > This change seems to modify normal C behavior again. Is there any strong 
> > motivation for doing this and if yes could it be done generically with C?
>
>
> Motivation:
>
>   // Non-portable OpenCL 1.2 code
>   __kernel void foo(global float* out) {
> out[get_global_id(0)] = sin(get_global_id(0));
>   }
>
>
> This program compiles fine on OpenCL platform w/o doubles support

Perhaps that is the problem (that there are two modes that do different 
things)? Could we make the double overload be present but unselectable to 
diagnose this problem in that mode too?

and fails otherwise.
>  If OpenCL driver supports doubles it provides at least two versions of 'sin' 
> built-in math function and compiler will not be able to choose the right one 
> for 'size_t' argument.

Do you have a real example? If someone passes an integer to 'sin', I think 
there's a better warning we can give them :)

>  The goal of this warning is to let OpenCL developer know about potential 
> issues with code portability.

I would argue this improves the portability much as it can also be misleading 
in some situations (because it refers to a potentially hypothetical problem). 
For example there can be builtin functions that only have a float parameter 
(without a double version of it). This is for example the case with read_image 
functions that take a float coordinate value between 0 and 1. Unfortunately 
this warning won't be triggered on read_image functions because there is an 
overload candidate with an int type of the same parameter too. But we can't 
exclude this situations to appear in the future or from some vendor extensions 
or even custom OpenCL code.


https://reviews.llvm.org/D27334



___
cfe-commits mailing list
cfe-commits@lists.llvm.org<mailto:cfe-commits@lists.llvm.org>
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D27334: [OpenCL] Ambiguous function call.

[Richard Smith via cfe-commits]

On 5 Dec 2016 9:42 am, "Anastasia Stulova via Phabricator via cfe-commits" <
cfe-commits@lists.llvm.org> wrote:

Anastasia added a comment.

In https://reviews.llvm.org/D27334#612858, @bader wrote:

> In https://reviews.llvm.org/D27334#611703, @Anastasia wrote:
>
> > This change seems to modify normal C behavior again. Is there any
strong motivation for doing this and if yes could it be done generically
with C?
>
>
> Motivation:
>
>   // Non-portable OpenCL 1.2 code
>   __kernel void foo(global float* out) {
> out[get_global_id(0)] = sin(get_global_id(0));
>   }
>
>
> This program compiles fine on OpenCL platform w/o doubles support


Perhaps that is the problem (that there are two modes that do different
things)? Could we make the double overload be present but unselectable to
diagnose this problem in that mode too?

and fails otherwise.
>  If OpenCL driver supports doubles it provides at least two versions of
'sin' built-in math function and compiler will not be able to choose the
right one for 'size_t' argument.


Do you have a real example? If someone passes an integer to 'sin', I think
there's a better warning we can give them :)

>  The goal of this warning is to let OpenCL developer know about potential
issues with code portability.


I would argue this improves the portability much as it can also be
misleading in some situations (because it refers to a potentially
hypothetical problem). For example there can be builtin functions that only
have a float parameter (without a double version of it). This is for
example the case with read_image functions that take a float coordinate
value between 0 and 1. Unfortunately this warning won't be triggered on
read_image functions because there is an overload candidate with an int
type of the same parameter too. But we can't exclude this situations to
appear in the future or from some vendor extensions or even custom OpenCL
code.


https://reviews.llvm.org/D27334



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27334: [OpenCL] Ambiguous function call.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In https://reviews.llvm.org/D27334#612858, @bader wrote:

> In https://reviews.llvm.org/D27334#611703, @Anastasia wrote:
>
> > This change seems to modify normal C behavior again. Is there any strong 
> > motivation for doing this and if yes could it be done generically with C?
>
>
> Motivation:
>
>   // Non-portable OpenCL 1.2 code 
>   __kernel void foo(global float* out) {
> out[get_global_id(0)] = sin(get_global_id(0));
>   }
>
>
> This program compiles fine on OpenCL platform w/o doubles support and fails 
> otherwise.
>  If OpenCL driver supports doubles it provides at least two versions of 'sin' 
> built-in math function and compiler will not be able to choose the right one 
> for 'size_t' argument.
>  The goal of this warning is to let OpenCL developer know about potential 
> issues with code portability.


I would argue this improves the portability much as it can also be misleading 
in some situations (because it refers to a potentially hypothetical problem). 
For example there can be builtin functions that only have a float parameter 
(without a double version of it). This is for example the case with read_image 
functions that take a float coordinate value between 0 and 1. Unfortunately 
this warning won't be triggered on read_image functions because there is an 
overload candidate with an int type of the same parameter too. But we can't 
exclude this situations to appear in the future or from some vendor extensions 
or even custom OpenCL code.


https://reviews.llvm.org/D27334



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27334: [OpenCL] Ambiguous function call.

[Alexey Bader via Phabricator via cfe-commits]

bader added a comment.

In https://reviews.llvm.org/D27334#611703, @Anastasia wrote:

> This change seems to modify normal C behavior again. Is there any strong 
> motivation for doing this and if yes could it be done generically with C?


Motivation:

  // Non-portable OpenCL 1.2 code 
  __kernel void foo(global float* out) {
out[get_global_id(0)] = sin(get_global_id(0));
  }

This program compiles fine on OpenCL platform w/o doubles support and fails 
otherwise.
If OpenCL driver supports doubles it provides at least two versions of 'sin' 
built-in math function and compiler will not be able to choose the right one 
for 'size_t' argument.
The goal of this warning is to let OpenCL developer know about potential issues 
with code portability.

This might be not so serious issue for C, since C doesn't support function 
overloading, whereas OpenCL built-in functions library must overload most of 
the functions.


https://reviews.llvm.org/D27334



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27334: [OpenCL] Ambiguous function call.

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

This change seems to modify normal C behavior again. Is there any strong 
motivation for doing this and if yes could it be done generically with C?




Comment at: lib/Sema/SemaChecking.cpp:2479
+// integer values.
+if (FDecl->hasAttr()) {
+  for (const auto* Arg : Args) {

How does it check that this is a built-in function?


https://reviews.llvm.org/D27334



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27334: [OpenCL] Ambiguous function call.

[Egor Churaev via Phabricator via cfe-commits]

echuraev created this revision.
echuraev added a reviewer: Anastasia.
echuraev added subscribers: bader, cfe-commits, yaxunl.

Added warning about potential ambiguity error with built-in overloading.

Patch by Alexey Bader


https://reviews.llvm.org/D27334

Files:
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaChecking.cpp
  test/SemaOpenCL/warn-potential-abiguity.cl


Index: test/SemaOpenCL/warn-potential-abiguity.cl
===
--- /dev/null
+++ test/SemaOpenCL/warn-potential-abiguity.cl
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -pedantic 
-Wconversion-might-lead-to-ambiguity %s
+
+float __attribute__((overloadable)) ocl_builtin_func(float f) { return f; }
+float __attribute__((overloadable)) ocl_builtin_func_2args(float arg1, float 
arg2) { return arg1 + arg2; }
+
+__kernel void test() {
+  int p = ocl_builtin_func(3); // expected-warning {{implicit conversion from 
integral type to floating point type for overloadable function might lead to 
ambiguity}}
+  int q = ocl_builtin_func_2args(3.f, 3); // expected-warning {{implicit 
conversion from integral type to floating point type for overloadable function 
might lead to ambiguity}}
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -2472,6 +2472,19 @@
 if (FDecl) {
   for (const auto *I : FDecl->specific_attrs())
 CheckArgumentWithTypeTag(I, Args.data());
+
+  if (getLangOpts().OpenCL) {
+// Check if overloadble built-in function with floating point 
arguments takes
+// integer values.
+if (FDecl->hasAttr()) {
+  for (const auto* Arg : Args) {
+const ImplicitCastExpr *ICE = dyn_cast(Arg);
+if (!ICE || ICE->getCastKind() != CK_IntegralToFloating)
+  continue;
+Diag(Loc, diag::warn_ocl_bultin_potential_ambiguity) << Range;
+  }
+}
+  }
 }
   }
 }
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -8099,6 +8099,10 @@
   "missing actual type specifier for pipe">;
 def err_reference_pipe_type : Error <
   "pipes packet types cannot be of reference type">;
+def warn_ocl_bultin_potential_ambiguity : Warning<
+"implicit conversion from integral type to floating point type for"
+" overloadable function might lead to ambiguity">,
+  InGroup>, DefaultIgnore;
 def err_opencl_no_main : Error<"%select{function|kernel}0 cannot be called 
'main'">;
 def err_opencl_kernel_attr :
   Error<"attribute %0 can only be applied to a kernel function">;


Index: test/SemaOpenCL/warn-potential-abiguity.cl
===
--- /dev/null
+++ test/SemaOpenCL/warn-potential-abiguity.cl
@@ -0,0 +1,9 @@
+// RUN: %clang_cc1 -fsyntax-only -verify -pedantic -Wconversion-might-lead-to-ambiguity %s
+
+float __attribute__((overloadable)) ocl_builtin_func(float f) { return f; }
+float __attribute__((overloadable)) ocl_builtin_func_2args(float arg1, float arg2) { return arg1 + arg2; }
+
+__kernel void test() {
+  int p = ocl_builtin_func(3); // expected-warning {{implicit conversion from integral type to floating point type for overloadable function might lead to ambiguity}}
+  int q = ocl_builtin_func_2args(3.f, 3); // expected-warning {{implicit conversion from integral type to floating point type for overloadable function might lead to ambiguity}}
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -2472,6 +2472,19 @@
 if (FDecl) {
   for (const auto *I : FDecl->specific_attrs())
 CheckArgumentWithTypeTag(I, Args.data());
+
+  if (getLangOpts().OpenCL) {
+// Check if overloadble built-in function with floating point arguments takes
+// integer values.
+if (FDecl->hasAttr()) {
+  for (const auto* Arg : Args) {
+const ImplicitCastExpr *ICE = dyn_cast(Arg);
+if (!ICE || ICE->getCastKind() != CK_IntegralToFloating)
+  continue;
+Diag(Loc, diag::warn_ocl_bultin_potential_ambiguity) << Range;
+  }
+}
+  }
 }
   }
 }
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -8099,6 +8099,10 @@
   "missing actual type specifier for pipe">;
 def err_reference_pipe_type : Error <
   "pipes packet types cannot be of reference type">;
+def warn_ocl_bultin_potential_ambiguity : Warning<
+"implicit conversion from integral