[cfe-commits] (no subject)

[Douglas Gregor]

On Jan 18, 2013, at 9:26 AM, Joey Gouly <joey.gouly at arm.com> wrote:

> Hi all,
> 
> I am sending out our patch for fp16 as a native type (not storage only).
> 
> All the comments from Douglas have been addressed from the previous thread,
> and Timo recently brought up [1] that
> he is using this patch in pocl without problem.
> 
> Any comments? Can I commit it?


A few comments...

index 8ef766f..d58a273 100644
--- a/include/clang/Basic/LangOptions.def
+++ b/include/clang/Basic/LangOptions.def
@@ -116,6 +116,7 @@ LANGOPT(ShortEnums        , 1, 0, "short enum types")
 
 LANGOPT(OpenCL            , 1, 0, "OpenCL")
 LANGOPT(OpenCLVersion     , 32, 0, "OpenCL version")
+LANGOPT(NativeHalfType    , 1, 0, "Native half type support")
 LANGOPT(CUDA              , 1, 0, "CUDA")
 
I'd like to have "OpenCL" in the name, because these semantics are very much tied to OpenCL.

--- a/lib/Sema/SemaDecl.cpp
+++ b/lib/Sema/SemaDecl.cpp
@@ -4288,6 +4288,23 @@ Sema::ActOnVariableDeclarator(Scope *S, Declarator &D, DeclContext *DC,
   assert(SCSpec != DeclSpec::SCS_typedef &&
          "Parser allowed 'typedef' as storage class VarDecl.");
   VarDecl::StorageClass SC = StorageClassSpecToVarDeclStorageClass(SCSpec);
+
+  if (getLangOpts().OpenCL && !getOpenCLOptions().cl_khr_fp16)
+  {
+    // If the cl_khr_fp16 extension is disabled, reject declaring variables
+    // of the half and half array type (OpenCL 1.1 6.1.1.1).
+    bool isHalfType = false;
+    if (R->isHalfType()) {
+      isHalfType = true;
+    } else if (const ArrayType *ArrayTy = Context.getAsArrayType(R)) {
+      isHalfType = ArrayTy->getElementType()->isHalfType();
+    }
+    if (isHalfType) {
+      Diag(D.getIdentifierLoc(), diag::err_opencl_half_declaration) << R;
+      D.setInvalidType();
+    }
+  }

How about using ASTContext::getBaseElementType()?

--- a/lib/Sema/SemaType.cpp
+++ b/lib/Sema/SemaType.cpp
@@ -2403,9 +2403,15 @@ static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state,
       // Do not allow returning half FP value.
       // FIXME: This really should be in BuildFunctionType.
       if (T->isHalfType()) {
-        S.Diag(D.getIdentifierLoc(),
-             diag::err_parameters_retval_cannot_have_fp16_type) << 1
-          << FixItHint::CreateInsertion(D.getIdentifierLoc(), "*");
+        if (S.getLangOpts().OpenCL) {
+          if (!S.getOpenCLOptions().cl_khr_fp16)
+            S.Diag(D.getIdentifierLoc(), diag::err_opencl_half_return) << T
+              << FixItHint::CreateInsertion(D.getIdentifierLoc(), "*");
+        } else {
+          S.Diag(D.getIdentifierLoc(),
+            diag::err_parameters_retval_cannot_have_fp16_type) << 1
+            << FixItHint::CreateInsertion(D.getIdentifierLoc(), "*");
+        }
         D.setInvalidType(true);
       }
 
Whenever we provide a Fix-It, we need to recover as if the user had written what we're suggesting. So if we're going to have to actually turn this into a pointer-to-half type or remove the Fix-It. It's unfortunate the code didn't follow this approach previously; we must have missed it in review.

--- a/lib/Sema/SemaType.cpp
+++ b/lib/Sema/SemaType.cpp
@@ -2403,9 +2403,15 @@ static TypeSourceInfo *GetFullTypeForDeclarator(TypeProcessingState &state,
       // Do not allow returning half FP value.
       // FIXME: This really should be in BuildFunctionType.
       if (T->isHalfType()) {
-        S.Diag(D.getIdentifierLoc(),
-             diag::err_parameters_retval_cannot_have_fp16_type) << 1
-          << FixItHint::CreateInsertion(D.getIdentifierLoc(), "*");
+        if (S.getLangOpts().OpenCL) {
+          if (!S.getOpenCLOptions().cl_khr_fp16)
+            S.Diag(D.getIdentifierLoc(), diag::err_opencl_half_return) << T
+              << FixItHint::CreateInsertion(D.getIdentifierLoc(), "*");
+        } else {
+          S.Diag(D.getIdentifierLoc(),
+            diag::err_parameters_retval_cannot_have_fp16_type) << 1
+            << FixItHint::CreateInsertion(D.getIdentifierLoc(), "*");
+        }
         D.setInvalidType(true);
       }
 
Same comment about Fix-Its.

	- Doug




	- Doug