RE: [PATCH] D60455: [SYCL] Implement SYCL device code outlining

[Bader, Alexey via cfe-commits]

Hi Aaron,

I think Anastasia is on vacation.
Could you recommend someone who can be “trusted reviewer” for this change, 
please?
If no, we will wait for your/Anastasia review.

Thanks,
Alexey

From: Aaron Ballman 
Sent: Tuesday, June 11, 2019 5:04 PM
To: reviews+d60455+public+bc9d5bb3412ac...@reviews.llvm.org
Cc: Podchishchaeva, Mariya ; Justin Lebar 
; ronan-l...@keryell.fr; v.lomul...@gmail.com; 
a.bat...@hotmail.com; anastasia.stul...@arm.com; Bader, Alexey 
; mgo...@gentoo.org; Maslov, Oleg 
; Gainullin, Artur ; 
b00234...@studentmail.uws.ac.uk; bevin.hans...@ericsson.com; 
cfe-commits@lists.llvm.org; mlek...@skidmore.edu; blitzrak...@gmail.com; 
shen...@google.com
Subject: Re: [PATCH] D60455: [SYCL] Implement SYCL device code outlining

I'm out of the office until next week and won't be able to review it until 
then. If another trusted reviewer accepts, I can always do post commit review.

~Aaron

On Tue, Jun 11, 2019, 2:43 PM Mariya Podchishchaeva via Phabricator 
mailto:revi...@reviews.llvm.org>> wrote:
Fznamznon added a comment.

@aaron.ballman , please let me know if you have additional 
comments/suggestions. If not, could you please accept this revision?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60455/new/

https://reviews.llvm.org/D60455


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://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 ; Bruno Cardoso Lopes via Phabricator 
; 
reviews+d27334+public+f2c5a66032c4c...@reviews.llvm.org
Cc: Bader, Alexey ; egor.chur...@gmail.com; 
yaxun@amd.com; cfe-commits@lists.llvm.org; nd 
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] 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" 
> 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



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

This 

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