Page MenuHomePhabricator

[OpenCL] Allow half type kernel argument when cl_khr_fp16 is enabled
ClosedPublic

Authored by yaxunl on Sep 16 2016, 7:32 AM.

Details

Reviewers
bader

Diff Detail

Event Timeline

yaxunl updated this revision to Diff 71642.Sep 16 2016, 7:32 AM
yaxunl retitled this revision from to [OpenCL] Allow half type kernel argument when cl_khr_fp16 is enabled.
yaxunl updated this object.
yaxunl added reviewers: Anastasia, bader.
yaxunl added subscribers: cfe-commits, nhaustov, rampitec.
bader added inline comments.Sep 16 2016, 8:21 AM
test/SemaOpenCL/half.cl
29

It's not related to your change, but it looks like there are two checks that report the same error.
Could you remove the duplication, please?

yaxunl updated this revision to Diff 71667.Sep 16 2016, 9:34 AM

Remove redundant diagnostic msg.

bader added inline comments.Sep 16 2016, 11:11 AM
lib/Sema/SemaDecl.cpp
7599–7602

It looks strange to me. First we check if parameter type is half - we set InvalidKernelParam status, later we check again and do not report an error.
I think it would be simpler just return ValidKernelParam for half data type from getOpenCLKernelParameterType,

I think the whole patch should two deleted lines from getOpenCLKernelParameterType function + test case.

yaxunl added inline comments.Sep 16 2016, 11:20 AM
lib/Sema/SemaDecl.cpp
7599–7602

getOpenCLKernelParameterType should report half type as InvalidKernelParam since it really is an invalid kernel argument type. We do not emit diagnostic msg because the msg is redundant, not because half type is a valid kernel argument type.

getOpenCLKernelParameterType may be used for other purpose. Reporting half type as a valid kernel argument violates the semantics of getOpenCLKernelParameterType and can cause confusion and potential error.

Anastasia added inline comments.Sep 17 2016, 12:49 PM
lib/Sema/SemaDecl.cpp
7600

-> elsewhere

Anastasia added a subscriber: Anastasia.
bader added inline comments.Sep 19 2016, 8:26 AM
lib/Sema/SemaDecl.cpp
7599–7602

Maybe we should the other way.
Leave half parameter check here only and remove duplicate check that reports "declaring function parameter of type 'half' is not allowed; did you forget * ?" message.

yaxunl added inline comments.Sep 19 2016, 9:18 AM
lib/Sema/SemaDecl.cpp
7599–7602

Clang can emit two error msgs in this case since it does two independent checks:

  1. half type cannot be used as function argument when cl_khr_fp16 is disabled
  1. half type cannot be used as kernel function argument when cl_khr_fp16 is disabled

I think error msg 1 is better than error msg 2 since error msg 1 provides more information, i.e., when cl_khr_fp16 is disabled, half type cannot be used as either kernel function argument or non-kernel function argument, whereas error msg 2 may give user the wrong impression that half type can not be used for kernel function argument only.

bader accepted this revision.Sep 19 2016, 9:23 AM
bader edited edge metadata.
This revision is now accepted and ready to land.Sep 19 2016, 9:23 AM
yaxunl marked 6 inline comments as done.Sep 19 2016, 10:18 AM
yaxunl added inline comments.
lib/Sema/SemaDecl.cpp
7600

I will fix it in commit.

yaxunl closed this revision.Sep 19 2016, 10:24 AM
yaxunl marked an inline comment as done.