[cfe-dev] OpenCL support

[Chris Lattner]

On Jan 18, 2011, at 9:24 AM, Anton Lokhmotov wrote:

> Hi Peter,
> 
> We've made the changes to the first patch that we discussed.  Please review.

Hi Anton,

I'm coming late to the review on this patch, but I have a couple of concerns:

+++ b/include/clang/Basic/TokenKinds.def
+// OpenCL-specific keywords (see OpenCL 1.1 [6.1.9])
+KEYWORD(__kernel                    , KEYOPENCL)
+ALIAS("kernel", __kernel            , KEYOPENCL)


Why define these as keywords in the language?  OpenCL requires an implicit prefix header to be included (which has all the conversion functions and a bunch of other stuff).  It seems cleaner to just put a #define in this header that defines __kernel to expand to an attribute.

Doing this would remove the parser hook.


+  if(Tok.isNot(tok::colon)) {
+    //TODO: error message
+    return;
+  }

Please put a space after control flow keywords: if (Tok.isNot ...

Also, please do implement the error messages.

+#    define OPENCLEXT(nm)   f.nm = on;
+#    include "clang/Basic/OpenCLExtensions.def"
+  }
+#    define OPENCLEXT(nm) else if(ename->isStr(#nm)) { f.nm = on; }
+#    include "clang/Basic/OpenCLExtensions.def"

Please don't use spaces after the #.

+void PragmaOpenCLFPContractHandler::HandlePragma(Preprocessor &PP, 
+  PragmaIntroducerKind Introducer,
+  Token &CLTok) 
+{
+  SourceLocation CLLoc = CLTok.getLocation();
+
+  Token Tok;
+  PP.Lex(Tok);
+  if (Tok.isNot(tok::identifier)) {
+    PP.Diag(Tok.getLocation(), diag::warn_pragma_expected_identifier)
+      << "OPENCL";
+    return;
+  }
+
+  ParseFPContractPragma(Actions, PP);
+  return;
+
+
+}

Please put the { for the function on the same line as the function, and indent the parameters to match the rest of the file.  Also, no need for an explicit "return" at the end and the whitespace after it.


+static void ParseFPContractPragma(Sema &Actions, Preprocessor &PP)
+{
+  Token T1;
+  PP.Lex(T1);
+  if (T1.isNot(tok::identifier)) {
+    PP.Diag(T1.getLocation(), diag::warn_pragma_expected_identifier)
+      << "FP_CONTRACT";
+    return;
+  }
+  IdentifierInfo *op = T1.getIdentifierInfo();
+  if(op->isStr("ON") || 
+     (Actions.getLangOptions().OpenCL && op->isStr("DEFAULT"))) {
+    // FP_CONTRACT is ON
+    Actions.getFPOptions().fp_contract = 1;
+    return;
+  } else if(op->isStr("OFF")) {
+    // FP_CONTRACT is OFF
+    Actions.getFPOptions().fp_contract = 0;
+    return;
+  } else {
+    PP.Diag(T1.getLocation(), diag::warn_pragma_expected_identifier) << 
+      "FP_CONTRACT";
+    return;
+  }
+}

There is no need for "else after return".


+++ b/lib/Parse/Parser.cpp
+
+  if(getLang().OpenCL) {
+    OpenCLExtensionHandler.reset(new PragmaOpenCLExtensionHandler(actions,*this));
+    PP.AddPragmaHandler("OPENCL",OpenCLExtensionHandler.get());
+    OpenCLFPContractHandler.reset(new PragmaOpenCLFPContractHandler(actions,*this));
+    PP.AddPragmaHandler("OPENCL",OpenCLFPContractHandler.get());
+  }

Please fit to 80 columns.

+++ b/lib/CodeGen/CodeGenFunction.cpp
@@ -236,6 +236,24 @@ void CodeGenFunction::StartFunction(GlobalDecl GD, QualType RetTy,
         break;
       }
 
+  if(getContext().getLangOptions().OpenCL) {
+    // Toggle a kernel specification, if it's set for any declaration
+    if (const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(D))
+      for (FunctionDecl::redecl_iterator RI = FD->redecls_begin(),
+             RE = FD->redecls_end(); RI != RE; ++RI)
+        if (RI->hasAttr<OpenCLKernelAttr>()) {       

I'm not an expert on this stuff, but it seems like there should be a better way to see if a function has an attribute.  Please end your comment with a ".".  This isn't really "Toggling" it, it is adding metadata for it.

+          llvm::LLVMContext &Context = getLLVMContext();
+          llvm::NamedMDNode *OpenCLMetadata = 
+            CGM.getModule().getOrInsertNamedMetadata("opencl.kernels");
+          
+          llvm::Value *Ops[] = { Fn };
+          
+          OpenCLMetadata->addOperand(llvm::MDNode::get(Context, Ops, 1));
+          
+          break;

You can simplify this to:

  llvm::Value *Op = Fn;
  OpenCLMetadata->addOperand(llvm::MDNode::get(Context, &Op, 1));
  break;

-Chris