OpenCL support

Anton_Lokhmotov · December 10, 2010, 6:21pm

Please review our first patch for OpenCL support in Clang. It covers
several interrelated features that we found hard to break into smaller
patches:
- kernel qualifier
- pragmas
- extensions
This patch will be followed by relevant test cases.

I would like to discuss couple of points.

1) For kernel functions (with the __kernel qualifier), we insert named
metadata nodes into LLVM-IR. Other options we have evaluated included:
- Shipping metadata separately from bitcode. This would be hard to
standardise on and maintain.
- Using imaginative name mangling. This would be pretty horrible too and
easy to misuse.

The drawback of course is that metadata can be "silently dropped". However,
we are not aware of any transforms that would do this. Perhaps the
community should consider defining "sticky metadata"?

2) We've introduced a new function *getLangOptionsPtrUnsafe(), because the
OpenCL pragmas handler must be able to modify the language options for the
current context, and LangOptions structure is the most natural place to
store the OpenCL EXTENSIONS and FP_CONTRACT states. Given such a use,
perhaps constness should be taken away from the getLangOptions()?

Please review and comment.

Many thanks,
Anton.

000100-kernel-extensions-pragmas.patch (17.8 KB)

Mike_Gist · December 10, 2010, 11:18pm

For kernel functions (with the __kernel qualifier), we insert named
metadata nodes into LLVM-IR. Other options we have evaluated included:

Shipping metadata separately from bitcode. This would be hard to
standardise on and maintain.

Using imaginative name mangling. This would be pretty horrible too and
easy to misuse.

The drawback of course is that metadata can be “silently dropped”. However,
we are not aware of any transforms that would do this. Perhaps the
community should consider defining “sticky metadata”?

You could also consider placing all kernel functions in a ‘kernel’ section, or
adding a function attribute for kernels.

Inserting metadata feels a little dirty to me, it shouldn’t be used if it would change
the semantics of the program (which it would in this case). Some kind of ‘sticky’ metadata
would indeed get around ‘dropping’, however I don’t believe (I may be wrong) that metadata was
designed for this purpose. I’m not sure that trying to fit it to this purpose is a good idea.

Please review and comment.

cl_khr_byte_addressable_store is probably a fairly well supported 1.0 extension that
you could add too Those will have to be disabled by default for 1.0 anyhow.

Regards,
Mike

pcc · December 12, 2010, 11:44pm

The current PTX backend uses a target-specific calling convention for
kernel functions (CallingConv::PTX_Kernel). Another approach we may
consider is to standardise a kernel calling convention across backends.

Thanks,

pcc · December 13, 2010, 12:29am

Hi Anton,

2) We've introduced a new function *getLangOptionsPtrUnsafe(), because the
OpenCL pragmas handler must be able to modify the language options for the
current context, and LangOptions structure is the most natural place to
store the OpenCL EXTENSIONS and FP_CONTRACT states. Given such a use,
perhaps constness should be taken away from the getLangOptions()?

I don't think it is a good idea to use LangOptions for this.
LangOptions is an input parameter so we shouldn't modify it during
parsing/semantic analysis. Modifying the LangOptions could also affect
clients which reuse a LangOptions object expecting it to be unchanged.
The approach taken for other pragmas that affect semantic analysis is
to store data in the Sema object, and I think that is what we should
do here. For example, look at how "#pragma pack" is implemented.

Please review and comment.

Here are some initial comments:

@@ -1192,6 +1192,8 @@ private:
   bool IsTrivial : 1; // sunk from CXXMethodDecl
   bool HasImplicitReturnZero : 1;

+ bool IsOpenCLKernel : 1;
+
   /// \brief End part of this FunctionDecl's source range.
   ///
   /// We could compute the full range in getSourceRange(). However, when we're

A better way of storing this information would be to use an attribute
(Attr subclass). See the include/clang/Basic/Attr.td file.

We should only add a new field if we expect it to be used often.
Fields are quick to access but they increase the size of every
FunctionDecl object (we're using only 12 bits at the moment but may
wish to use more in future). The __kernel qualifier is a relatively
uncommon qualifier and does not need to be accessed often so it should
be added as an attribute.

@@ -230,7 +232,7 @@ KEYWORD(__func__ , KEYALL)

// C++ 2.11p1: Keywords.
KEYWORD(asm , KEYCXX|KEYGNU)
-KEYWORD(bool , BOOLSUPPORT|KEYALTIVEC)
+KEYWORD(bool , BOOLSUPPORT|KEYALTIVEC|KEYOPENCL)
KEYWORD(catch , KEYCXX)
KEYWORD(class , KEYCXX)
KEYWORD(const_cast , KEYCXX)
@@ -238,7 +240,7 @@ KEYWORD(delete , KEYCXX)
KEYWORD(dynamic_cast , KEYCXX)
KEYWORD(explicit , KEYCXX)
KEYWORD(export , KEYCXX)
-KEYWORD(false , BOOLSUPPORT|KEYALTIVEC)
+KEYWORD(false , BOOLSUPPORT|KEYALTIVEC|KEYOPENCL)
KEYWORD(friend , KEYCXX)
KEYWORD(mutable , KEYCXX)
KEYWORD(namespace , KEYCXX)
@@ -252,7 +254,7 @@ KEYWORD(static_cast , KEYCXX)
KEYWORD(template , KEYCXX)
KEYWORD(this , KEYCXX)
KEYWORD(throw , KEYCXX)
-KEYWORD(true , BOOLSUPPORT|KEYALTIVEC)
+KEYWORD(true , BOOLSUPPORT|KEYALTIVEC|KEYOPENCL)
KEYWORD(try , KEYCXX)
KEYWORD(typename , KEYCXX)
KEYWORD(typeid , KEYCXX)

Isn't this rendered unnecessary by this code? (from
lib/Frontend/CompilerInvocation.cpp):

// OpenCL and C++ both have bool, true, false keywords.
Opts.Bool = Opts.OpenCL || Opts.CPlusPlus;

Thanks,