Page MenuHomePhabricator

[HEADER] Overloadable function candidates for half/double types
Needs ReviewPublic

Authored by sidorovd on Aug 28 2018, 1:55 AM.

Details

Reviewers
yaxunl
Anastasia
Summary

Overloadable function candidate should not be viable if it uses extensions (Half or Double type) that are not enabled or supported.

Patch by Egor Churaev

Diff Detail

Event Timeline

sidorovd created this revision.Aug 28 2018, 1:55 AM
sidorovd updated this revision to Diff 162812.Aug 28 2018, 2:03 AM
krisb added a subscriber: krisb.Aug 28 2018, 2:27 AM
krisb removed a subscriber: krisb.
krisb added a subscriber: krisb.
Anastasia added inline comments.Aug 29 2018, 6:47 AM
lib/Sema/SemaOverload.cpp
6063

I would imagine if extension isn't supported the candidate function with data type defined by extension shouldn't be available at all during compilation?

Also is there any good way to generalize this for all types and extensions including vendor ones that are added with the pragmas?
https://clang.llvm.org/docs/UsersManual.html#opencl-extensions

test/SemaOpenCL/half-double-overload.cl
8

I think it will be clearer if candidates with non-half parameters moved out of extension enabled block.

19

Wondering if better message could be:

candidate unavailable as it requires OpenCL extension to be disabled

Could it also print the extension name?

sidorovd marked an inline comment as done.Sep 6 2018, 1:52 AM
sidorovd added inline comments.
lib/Sema/SemaOverload.cpp
6063

I would imagine if extension isn't supported the candidate function with data type defined by extension shouldn't be available at all during compilation?

Yes, you are right.

Also is there any good way to generalize this for all types and extensions including vendor ones that are added with the pragmas?

https://clang.llvm.org/docs/UsersManual.html#opencl-extensions

There might be a way but I can't properly answer this question right now. By default types and extensions aren't associated to each other.

test/SemaOpenCL/half-double-overload.cl
19

Sounds good. And I'd prefer to split it into another patch, because it would affect unrelated to the change tests and also require changes in Sema to allow extension name printing.

sidorovd updated this revision to Diff 164169.Sep 6 2018, 1:54 AM
sidorovd edited the summary of this revision. (Show Details)
Anastasia added inline comments.Sep 10 2018, 10:36 AM
lib/Sema/SemaOverload.cpp
6063

We have a map of types and associated extensions with them in OpenCLTypeExtMap in Sema.h... not sure what would be the cost of such generic check though.

test/SemaOpenCL/half-double-overload.cl
19

Ok, makes sense for this to be a separate change!

For now this patch is on hold since it adds no new functionality. Its future will be decided after @asavonic finished https://reviews.llvm.org/D51544

sidorovd added a comment.EditedFri, Nov 16, 5:51 AM

@Anastasia @yaxunl
Hi, I am working on generalizing this patch and several questions have raised during this, so I want to discuss them with you:

  1. Should #pragma OPENCL EXTENSION ext_name : begin enables the extension as well? For now I see it's not, as an example:
#pragma OPENCL EXTENSION cl_khr_fp16 : enable
half __attribute__((overloadable)) goo(half in1, half in2); // all ok
#pragma OPENCL EXTENSION cl_khr_fp16 : disable

#pragma OPENCL EXTENSION cl_khr_fp16 : begin
half __attribute__((overloadable)) goo(half in1, half in2); // declaring function parameter of type 'half' is not allowed; did you forget * ?
#pragma OPENCL EXTENSION cl_khr_fp16 : end
  1. As far as I understand, when declaring an extension we shall have 1 #pragma begin and 1 #pragma end. But here is a test called extension-begin and in its header one can see this construction:
#pragma OPENCL EXTENSION all : begin
#pragma OPENCL EXTENSION all : end 

#pragma OPENCL EXTENSION my_ext : begin
///some code
#pragma OPENCL EXTENSION my_ext : end 
#pragma OPENCL EXTENSION my_ext : end // why?
}

so here my_ext has double ending. And in this way the test passes, but if I remove second ending (which is redundant from my perspective), I see following diagnostics: " OpenCL extension end directive mismatches begin directive - ignoring". Is it a bug or it's supposed to work that way?

@Anastasia @yaxunl
Hi, I am working on generalizing this patch and several questions have raised during this, so I want to discuss them with you:

  1. Should #pragma OPENCL EXTENSION ext_name : begin enables the extension as well? For now I see it's not, as an example:

    ` #pragma OPENCL EXTENSION cl_khr_fp16 : enable half attribute((overloadable)) goo(half in1, half in2); // all ok #pragma OPENCL EXTENSION cl_khr_fp16 : disable

    #pragma OPENCL EXTENSION cl_khr_fp16 : begin half attribute((overloadable)) goo(half in1, half in2); // declaring function parameter of type 'half' is not allowed; did you forget * ? #pragma OPENCL EXTENSION cl_khr_fp16 : end `

#pragma OPENCL EXTENSION ext_name : begin should not enable the extension. There are cases that you want to declare functions and types associated with an extension but do not want to enable the extension.

  1. As far as I understand, when declaring an extension we shall have 1 #pragma begin and 1 #pragma end. But here is a test called extension-begin and in its header one can see this construction:

    ` #pragma OPENCL EXTENSION all : begin #pragma OPENCL EXTENSION all : end

    #pragma OPENCL EXTENSION my_ext : begin /some code #pragma OPENCL EXTENSION my_ext : end #pragma OPENCL EXTENSION my_ext : end why? } ` so here my_ext has double ending. And in this way the test passes, but if I remove second ending (which is redundant from my perspective), I see following diagnostics: " OpenCL extension end directive mismatches begin directive - ignoring". Is it a bug or it's supposed to work that way?

It seems to be a bug.