Re: [PATCH] D24666: [OpenCL] Allow half type kernel argument when cl_khr_fp16 is enabled
yaxunl marked 6 inline comments as done. Comment at: lib/Sema/SemaDecl.cpp:7600 @@ +7599,3 @@ +// Do not diagnose half type since it is diagnosed as invalid argument +// type for any function eleswhere. +if (!PT->isHalfType()) Anastasia wrote: > -> elsewhere I will fix it in commit. https://reviews.llvm.org/D24666 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24666: [OpenCL] Allow half type kernel argument when cl_khr_fp16 is enabled
yaxunl added inline comments. Comment at: lib/Sema/SemaDecl.cpp:7599-7602 @@ -7595,3 +7598,6 @@ // of event_t type. -S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT; +// Do not diagnose half type since it is diagnosed as invalid argument +// type for any function eleswhere. +if (!PT->isHalfType()) + S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT; D.setInvalidType(); bader wrote: > yaxunl wrote: > > bader wrote: > > > 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. > > 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. > 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. 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 2. 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. https://reviews.llvm.org/D24666 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24666: [OpenCL] Allow half type kernel argument when cl_khr_fp16 is enabled
bader added inline comments. Comment at: lib/Sema/SemaDecl.cpp:7599-7602 @@ -7595,3 +7598,6 @@ // of event_t type. -S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT; +// Do not diagnose half type since it is diagnosed as invalid argument +// type for any function eleswhere. +if (!PT->isHalfType()) + S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT; D.setInvalidType(); yaxunl wrote: > bader wrote: > > 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. > 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. 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. https://reviews.llvm.org/D24666 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24666: [OpenCL] Allow half type kernel argument when cl_khr_fp16 is enabled
Anastasia added inline comments. Comment at: lib/Sema/SemaDecl.cpp:7600 @@ +7599,3 @@ +// Do not diagnose half type since it is diagnosed as invalid argument +// type for any function eleswhere. +if (!PT->isHalfType()) -> elsewhere https://reviews.llvm.org/D24666 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24666: [OpenCL] Allow half type kernel argument when cl_khr_fp16 is enabled
yaxunl added inline comments. Comment at: lib/Sema/SemaDecl.cpp:7599-7602 @@ -7595,3 +7598,6 @@ // of event_t type. -S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT; +// Do not diagnose half type since it is diagnosed as invalid argument +// type for any function eleswhere. +if (!PT->isHalfType()) + S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT; D.setInvalidType(); bader wrote: > 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. 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. https://reviews.llvm.org/D24666 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24666: [OpenCL] Allow half type kernel argument when cl_khr_fp16 is enabled
bader added inline comments. Comment at: lib/Sema/SemaDecl.cpp:7599-7602 @@ -7595,3 +7598,6 @@ // of event_t type. -S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT; +// Do not diagnose half type since it is diagnosed as invalid argument +// type for any function eleswhere. +if (!PT->isHalfType()) + S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT; D.setInvalidType(); 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. https://reviews.llvm.org/D24666 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24666: [OpenCL] Allow half type kernel argument when cl_khr_fp16 is enabled
yaxunl updated this revision to Diff 71667.
yaxunl added a comment.
Remove redundant diagnostic msg.
https://reviews.llvm.org/D24666
Files:
lib/Sema/SemaDecl.cpp
test/SemaOpenCL/half.cl
test/SemaOpenCL/invalid-kernel-parameters.cl
Index: test/SemaOpenCL/invalid-kernel-parameters.cl
===
--- test/SemaOpenCL/invalid-kernel-parameters.cl
+++ test/SemaOpenCL/invalid-kernel-parameters.cl
@@ -1,5 +1,7 @@
// RUN: %clang_cc1 -fsyntax-only -verify %s -triple spir-unknown-unknown
+kernel void half_arg(half x) { } // expected-error{{declaring function
parameter of type 'half' is not allowed; did you forget * ?}}
+
#pragma OPENCL EXTENSION cl_khr_fp16 : enable
@@ -11,7 +13,8 @@
kernel void bool_arg(bool x) { } // expected-error{{'bool' cannot be used as
the type of a kernel parameter}}
-kernel void half_arg(half x) { } // expected-error{{'half' cannot be used as
the type of a kernel parameter}}
+// half kernel argument is allowed when cl_khr_fp16 is enabled.
+kernel void half_arg(half x) { }
typedef struct ContainsBool // expected-note{{within field of type
'ContainsBool' declared here}}
{
Index: test/SemaOpenCL/half.cl
===
--- test/SemaOpenCL/half.cl
+++ test/SemaOpenCL/half.cl
@@ -25,6 +25,9 @@
return h;
}
+kernel void half_disabled_kernel(global half *p,
+ half h); // expected-error{{declaring
function parameter of type 'half' is not allowed; did you forget * ?}}
+
// Exactly the same as above but with the cl_khr_fp16 extension enabled.
#pragma OPENCL EXTENSION cl_khr_fp16 : enable
constant half a = 1.0h;
@@ -48,3 +51,7 @@
return h;
}
+
+kernel void half_enabled_kernel(global half *p,
+half h);
+
Index: lib/Sema/SemaDecl.cpp
===
--- lib/Sema/SemaDecl.cpp
+++ lib/Sema/SemaDecl.cpp
@@ -7524,7 +7524,7 @@
RecordKernelParam
};
-static OpenCLParamType getOpenCLKernelParameterType(QualType PT) {
+static OpenCLParamType getOpenCLKernelParameterType(Sema &S, QualType PT) {
if (PT->isPointerType()) {
QualType PointeeType = PT->getPointeeType();
if (PointeeType->isPointerType())
@@ -7545,7 +7545,10 @@
if (PT->isEventT())
return InvalidKernelParam;
- if (PT->isHalfType())
+ // OpenCL extension spec v1.2 s9.5:
+ // This extension adds support for half scalar and vector types as built-in
+ // types that can be used for arithmetic operations, conversions etc.
+ if (!S.getOpenCLOptions().cl_khr_fp16 && PT->isHalfType())
return InvalidKernelParam;
if (PT->isRecordType())
@@ -7566,7 +7569,7 @@
if (ValidTypes.count(PT.getTypePtr()))
return;
- switch (getOpenCLKernelParameterType(PT)) {
+ switch (getOpenCLKernelParameterType(S, PT)) {
case PtrPtrKernelParam:
// OpenCL v1.2 s6.9.a:
// A kernel function argument cannot be declared as a
@@ -7593,7 +7596,10 @@
// OpenCL v1.2 s6.8 n:
// A kernel function argument cannot be declared
// of event_t type.
-S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT;
+// Do not diagnose half type since it is diagnosed as invalid argument
+// type for any function eleswhere.
+if (!PT->isHalfType())
+ S.Diag(Param->getLocation(), diag::err_bad_kernel_param_type) << PT;
D.setInvalidType();
return;
@@ -7649,7 +7655,7 @@
if (ValidTypes.count(QT.getTypePtr()))
continue;
- OpenCLParamType ParamType = getOpenCLKernelParameterType(QT);
+ OpenCLParamType ParamType = getOpenCLKernelParameterType(S, QT);
if (ParamType == ValidKernelParam)
continue;
Index: test/SemaOpenCL/invalid-kernel-parameters.cl
===
--- test/SemaOpenCL/invalid-kernel-parameters.cl
+++ test/SemaOpenCL/invalid-kernel-parameters.cl
@@ -1,5 +1,7 @@
// RUN: %clang_cc1 -fsyntax-only -verify %s -triple spir-unknown-unknown
+kernel void half_arg(half x) { } // expected-error{{declaring function parameter of type 'half' is not allowed; did you forget * ?}}
+
#pragma OPENCL EXTENSION cl_khr_fp16 : enable
@@ -11,7 +13,8 @@
kernel void bool_arg(bool x) { } // expected-error{{'bool' cannot be used as the type of a kernel parameter}}
-kernel void half_arg(half x) { } // expected-error{{'half' cannot be used as the type of a kernel parameter}}
+// half kernel argument is allowed when cl_khr_fp16 is enabled.
+kernel void half_arg(half x) { }
typedef struct ContainsBool // expected-note{{within field of type 'ContainsBool' declared here}}
{
Index: test/SemaOpenCL/half.cl
===
--- test/SemaOpenCL/half.cl
+++ test/SemaOpenCL/half.cl
@@ -25,6 +25,9 @@
return h;
}
+kernel void half_disabled_kernel(global half *p,
+
Re: [PATCH] D24666: [OpenCL] Allow half type kernel argument when cl_khr_fp16 is enabled
bader added inline comments.
Comment at: test/SemaOpenCL/half.cl:29
@@ +28,3 @@
+kernel void half_disabled_kernel(global half *p,
+ half h); // expected-error{{'half' cannot be
used as the type of a kernel parameter}} // expected-error{{declaring function
parameter of type 'half' is not allowed; did you forget * ?}}
+
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?
https://reviews.llvm.org/D24666
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
