Page MenuHomePhabricator

[OpenCL] Ambiguous function call.
AbandonedPublic

Authored by echuraev on Dec 2 2016, 12:54 AM.

Details

Reviewers
Anastasia
Summary

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

Patch by Alexey Bader

Diff Detail

Event Timeline

echuraev updated this revision to Diff 80037.Dec 2 2016, 12:54 AM
echuraev retitled this revision from to [OpenCL] Ambiguous function call..
echuraev updated this object.
echuraev added a reviewer: Anastasia.
echuraev added subscribers: bader, cfe-commits, yaxunl.
Anastasia edited edge metadata.Dec 2 2016, 7:16 AM

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?

lib/Sema/SemaChecking.cpp
2479

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

bader added a comment.Dec 4 2016, 12:51 AM

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.

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.

bader added a comment.Dec 6 2016, 3:34 AM

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?

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.

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.

lib/Sema/SemaChecking.cpp
2479

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.

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?

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?

echuraev added a comment.EditedMay 10 2017, 11:25 PM

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?

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

echuraev abandoned this revision.May 22 2017, 5:52 AM