OpenCL & SPIR specific types - proposal and patch

I’d like to renew the discussion about making the OpenCL specific types first class citizens in Clang.

I think this change is required by the OpenCL specifications, since these type names are keywords of the OpenCL C language.

This change is also needed in order to enable efficient checking of OpenCL restrictions on these types (OpenCL 1.2 spec, section 6.9).

Furthermore, the proposed change will turn these types to pointers to opaque types, which means that it will hide the actual (vendor specific) implementation, so the OpenCL vendors using Clang will be able to implement these types in their own way.

This change would also be a basis for the implementation of SPIR generation by Clang. The SPIR discussion and spec can be found here:

http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-September/024132.html

http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-September/024178.html

Earlier discussion about the OpenCL types was started by Anton Lokhmotov:

http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-May/015297.html

http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-April/014741.html

http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-March/014118.html

http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-March/014121.html

image001.png

opencl_types.patch (25.4 KB)

Is it possible to unify the line endings on the patch(some lines have \lf and some have \cr\lf? From the OpenCL perspective this looks fine.

Micah

image001.png

Oops, sorry for the line ending issues. Apparently my svn client did some mess.

Attached the fixed patch.

Thanks

image001.png

opencl_types.patch (25.2 KB)

Hi Guy,

Basically, the patch looks good (after a quick look). Please find our
initial comments below.

Look forward to seeing more code for various restrictions on these data
types. We can help you there if needed.

Many thanks,
Anton.

+ CanQualType Image1dTTy, Image1dArrayTTy, Image1dBufferTTy;
+ CanQualType Image2dTTy, Image2dArrayTTy, Image3dTTy;

Please move Image3dTTy to a separate line for consistency.

+inline bool Type::isImageType() const {
+ return isImage1dT() || isImage1dArrayT() || isImage1dBufferT() ||
+ isImage2dT() || isImage2dArrayT() ||
+ isImage3dT();
+}

Start the check with 3d and 2d types (this will benefit OpenCL 1.1
compilers):

  return isImage3dT() || isImage2dT() || isImage2dArrayT() ||
         isImage1dT() || isImage1dArrayT() || isImage1dBufferT();

--- include/clang/Sema/DeclSpec.h (revision 165097)
+++ include/clang/Sema/DeclSpec.h (working copy)
   // type-qualifiers
@@ -306,7 +314,7 @@
   /*TSW*/unsigned TypeSpecWidth : 2;
   /*TSC*/unsigned TypeSpecComplex : 2;
   /*TSS*/unsigned TypeSpecSign : 2;
- /*TST*/unsigned TypeSpecType : 5;
+ /*TST*/unsigned TypeSpecType : 6;

We suspect that in addition to this change you also need to change:

--- a/include/clang/Basic/Specifiers.h
+++ b/include/clang/Basic/Specifiers.h
@@ -72,7 +72,7 @@ namespace clang {
   /// \brief Structure that packs information about the type specifiers
that
   /// were written in a particular type specifier sequence.
   struct WrittenBuiltinSpecs {
- /*DeclSpec::TST*/ unsigned Type : 5;
+ /*DeclSpec::TST*/ unsigned Type : 6;
     /*DeclSpec::TSS*/ unsigned Sign : 2;
     /*DeclSpec::TSW*/ unsigned Width : 2;
     bool ModeAttr : 1;

+ Width = Target->getPointerWidth(0); // Currently these types are
pointers
+ Align = Target->getPointerAlign(0); // to opaque types
      Width = Target->getPointerWidth(0); // Currently these types are
      Align = Target->getPointerAlign(0); // pointers to opaque types.

+ case tok::kw_image1d_t:
+ isInvalid = DS.SetTypeSpecType(DeclSpec::TST_image1d_t, Loc,
+ PrevSpec, DiagID);
+ break;
Please check indentation after this code. Sometimes there's an extra space
after 'Loc,'.

+ isInvalid = DS.SetTypeSpecType(DeclSpec::TST_image3d_t, Loc,
+ PrevSpec, DiagID);
PrevSpec is misaligned.

I'm hoping to have comments on this patch tomorrow, but since I have proposed several patches to Clang for the sampler type (and have another in revision).. can you explain why you want to change the type from an integer to a pointer?

-Tanya

There needs to be a way to differentiate between an integer and a sampler by only looking at the type. The sampler itself is an opaque type in OpenCL. The only requirement is that it is initialized with a 32bit unsigned integer, not that the type itself is an integer.

Micah

Hi Doug & Tanya,

Could you please have a look at this patch?
(see the description below as sent by Guy Benyei)

Thanks in advance,
Boaz

opencl_types.patch (25.2 KB)

I already responded about sampler briefly, but I'm not in favor of making sampler a pointer to an opaque type. I have more comments, but I need more than a day to write them up.

-Tanya

Anton,

I’ve fixed my patch according to your comments. I’ve also did some changes. See the patch attached.

Tanya,

I think the OpenCL specific types, including the sampler and the event types should be well distinguishable in IR format. The fact, that a 32 bit integer exposes a superset of the sampler functionality in some perspective is just a coincidence. The sampler type itself could contain additional information (for optimizations and other architecture specifics), and it doesn’t have to hold the initializer value in the bitwise or integer format.

The backend should be able to recognize the sampler type from the IR.

In the attached patch, I’ve moved the OpenCL specific type conversion to the CGOpenCLRuntime class – this class was meant to be overloaded by OpenCL vendors to implement vendor specific behavior (codegen for OpenCL locals is implemented there), and so if a vendor prefers using the i32 representation, he could implement it there.

In the next increment this class should also contain a sampler initialization function, so sampler initialization could be implemented also in a vendor specific way.

Please review this new patch

Thanks

image001.png

opencl_types2.patch (28.2 KB)

+BUILTIN_TYPE(Image1d_T, Image1dTTy)
+BUILTIN_TYPE(Image1dArray_T, Image1dArrayTTy)
+BUILTIN_TYPE(Image1dBuffer_T, Image1dBufferTTy)
+BUILTIN_TYPE(Image2d_T, Image2dTTy)
+BUILTIN_TYPE(Image2dArray_T, Image2dArrayTTy)
+BUILTIN_TYPE(Image3d_T, Image3dTTy)

The other builtin types corresponding to *_t keywords don’t have the ‘_T’ as part of the builtin name (Char16, Char32, WChar_S, WChar_U). Including the _T here is inconsistent. Also, how would you feel about giving these identifiers an OpenCL prefix, as we do for the ObjC builtin types?

  • bool isImage1dT() const; // OpenCL image1d_t
  • bool isImage1dArrayT() const; // OpenCL image1d_t
  • bool isImage1dBufferT() const; // OpenCL image1d_t
  • bool isImage2dT() const; // OpenCL image2d_t
  • bool isImage2dArrayT() const; // OpenCL image2d_t
  • bool isImage3dT() const; // OpenCL image3d_t

Some of these comments are wrong.

+inline bool Type::isImage1dT() const {

  • if (const BuiltinType *BT = dyn_cast(CanonicalType))
  • return BT->getKind() == BuiltinType::Image1d_T;
  • return false;
    +}

Please write this as “return isSpecificBuiltinType(BuiltinType::Image1d_T);”

  • Width = Target->getPointerWidth(0); // Currently these types are pointers

  • Align = Target->getPointerAlign(0); // to opaque types

Please put the comment by itself on the line before this (and add a full stop).

  • case BuiltinType::Event_T: Encoding = llvm::dwarf::DW_ATE_unsigned;

It seems that you’re giving these types the size and alignment of a pointer, but emitting debug info as if they’re ‘unsigned int’. Is that really the way this is supposed to work?

  • llvm::Type *ResultType;

Richard,

Thanks for your very helpful review.

Attached an updated patch – please review.

Thanks

image001.png

opencl_types3.patch (32.5 KB)

And now with unified line-endings…

image001.png

opencl_types3.patch (32.5 KB)

Hi Guy,

+inline bool Type::isImageType() const {
+ return isImage2dT() || isImage2dArrayT() ||
+ isImage3dT() ||
+ isImage1dT() || isImage1dArrayT() || isImage1dBufferT();
Any reason for isImage3dT() test not to be first?
*inline bool Type::isImageType() const {
* return isImage3dT() ||
* isImage2dT() || isImage2dArrayT() ||
* isImage1dT() || isImage1dArrayT() || isImage1dBufferT();

+inline bool Type::isOpenCLSpecificType() const {
+ return isSamplerT() || isEventT() ||
+ isImageType();
+}
*inline bool Type::isOpenCLSpecificType() const {
* return isSamplerT() || isEventT() || isImageType();
*}

+ // Currently these types are pointers to opaque types
* // Currently these types are pointers to opaque types.

+ llvm::DIType OCLImg1dTy, OCLImg1darrTy, OCLImg1dbuffTy;
+ llvm::DIType OCLImg2dTy, OCLImg2darrTy;
+ llvm::DIType OCLImg3dTy;
+ llvm::DIType OCLSmpTy;
+ llvm::DIType OCLEvtTy;
How about:
* llvm::DIType OCLImage1dDITy, OCLImage1dArrayDITy, OCLImage1dBufferDITy;
etc.

Cheers,
Anton.

There needs to be a way to differentiate between an integer and a sampler by only looking at the type. The sampler itself is an opaque type in OpenCL. The only requirement is that it is initialized with a 32bit unsigned integer, not that the type itself is an integer.

I understand that the spec doesn't require it to be a 32 bit integer, but if you are only ever assigning it a 32 bit integer, it doesn't make any sense to have the extra inttoptr instruction.

Secondly, your idea of differentiating an integer and sampler by type alone is then based upon the name of the type that you are pointing to and not actually the type itself. Anything based upon names can be risky. There was a point in time where the linker was not properly merging structures that were identical and therefore renaming them. It makes much more sense to attach metadata to samplers and images to denote what is what.

The image types being pointers I am fine with as there isn't any other way to do those.

-Tanya

I'm hoping to have comments on this patch tomorrow, but since I have proposed several patches to Clang for the sampler type (and have another in revision).. can you explain why you want to change the type from an integer to a pointer?

-Tanya

CGDebugInfo.cpp: Can you factor out the repeated code into a helper function?

image001.png

  • case BuiltinType::OCLEvent: {
  • if (OCLEvtTy.Verify())
  • return OCLEvtTy;
  • OCLEvtTy =
  • DBuilder.createForwardDecl(llvm::dwarf::DW_TAG_structure_type,
  • “opencl_event_t”, TheCU, getOrCreateMainFile(),
  • 0);
  • unsigned Size = CGM.getContext().getTypeSize(CGM.getContext().VoidPtrTy);
  • OCLEvtTy = DBuilder.createPointerType(OCLEvtTy, Size);
  • return OCLEvtTy;
  • }

Are these actually all pointer to struct forward declared types in the language that have no header that they’d normally be declared inside of?
Comments about these would be good, including the general layout.
Also yes, please try very hard to refactor these so that there is less duplicated code.

-eric

image001.png

Tanya,

I tend to agree with Micah:

Using i32 to represent samplers seems reasonable for most purposes, but in OpenCL sampler is not the same as int/uint. The only thing that really makes sampler_t look like uint is its initialization, but that’s it: one cannot do any arithmetic operations on samplers, can’t get the initializer value of samplers or pass samplers as integers and vice-versa.

Also, one can imagine implementations that would hold some optimization related info in the samplers: supposedly one could statically initialize a set of function pointers or other optimization data based on the initialize value – this initialization could be decided in compilation time. I guess this was the intention of the OpenCL spec, otherwise I’m not sure why are all the sampler specific restriction there – if the sampler was supposed to be a simple integer value, then one should be able to query its value, and do some bitwise operations to change the sampler behavior in runtime.

Please check the latest version I’ve sent – deriving from the CGOpenCLRuntime class there one could easily use i32 instead of these opaque type pointers for samplers.

Thanks

image001.png

Eric,

These are basically vendor specific types, their layout is not defined by the OpenCL spec or anywhere else. Each vendor can implement those in any way they like – they can be even replaced by non pointer types (like i32 for samplers and so on).

Thanks

image001.png

That sounds… interesting.

Ok. Please do refactor that code though.

-eric

image001.png