[cfe-dev] OpenCL & SPIR specific types - proposal and patch
Anton Lokhmotov
Anton.Lokhmotov at arm.com
Fri Oct 12 07:47:55 PDT 2012
Hi Guy,
+// OpenCL image types
+BUILTIN_TYPE(OCLImage1d, OCLImage1dTy)
+BUILTIN_TYPE(OCLImage1dArray, OCLImage1dArrayTy)
+BUILTIN_TYPE(OCLImage1dBuffer, OCLImage1dBufferTy)
+BUILTIN_TYPE(OCLImage2d, OCLImage2dTy)
+BUILTIN_TYPE(OCLImage2dArray, OCLImage2dArrayTy)
+BUILTIN_TYPE(OCLImage3d, OCLImage3dTy)
+
+// OpenCL sampler_t
+BUILTIN_TYPE(OCLSampler, OCLSamplerTy)
+
+// OpenCL event_t
+BUILTIN_TYPE(OCLEvent, OCLEventTy)
+
Please terminate the comments with dots.
This looks a bit weird:
+inline bool Type::isOpenCLSpecificType() const {
+ return isSamplerT() || isEventT() || isImageType();
+}
Shouldn't the image type indicator function be called isImageT()?
Thanks,
Anton.
From: Benyei, Guy [mailto:guy.benyei at intel.com]
Sent: 11 October 2012 15:49
To: cfe-dev at cs.uiuc.edu
Cc: Anton Lokhmotov; Benyei, Guy; Richard Smith; Ouriel, Boaz
Subject: RE: [cfe-dev] OpenCL & SPIR specific types - proposal and patch
Are there any additional comments about this patch?
Thanks
From: cfe-dev-bounces at cs.uiuc.edu [mailto:cfe-dev-bounces at cs.uiuc.edu] On
Behalf Of Benyei, Guy
Sent: Tuesday, October 09, 2012 22:49
To: Richard Smith
Cc: cfe-dev at cs.uiuc.edu; Anton.Lokhmotov at arm.com
Subject: Re: [cfe-dev] OpenCL & SPIR specific types - proposal and patch
I’ve refactored the code to remove the repeated parts, and also fixed Anton
Lokhmotov’s comments.
Please review.
Thanks
From: metafoo at gmail.com [mailto:metafoo at gmail.com] On Behalf Of Richard
Smith
Sent: Tuesday, October 09, 2012 21:26
To: Benyei, Guy
Cc: cfe-dev at cs.uiuc.edu; Villmow, Micah; Tanya Lattner;
Anton.Lokhmotov at arm.com
Subject: Re: [cfe-dev] OpenCL & SPIR specific types - proposal and patch
CGDebugInfo.cpp: Can you factor out the repeated code into a helper
function?
On Tue, Oct 9, 2012 at 4:36 AM, Benyei, Guy <guy.benyei at intel.com> wrote:
And now with unified line-endings…
From: cfe-dev-bounces at cs.uiuc.edu [mailto:cfe-dev-bounces at cs.uiuc.edu] On
Behalf Of Benyei, Guy
Sent: Tuesday, October 09, 2012 13:26
To: Richard Smith
Cc: cfe-dev at cs.uiuc.edu; Anton.Lokhmotov at arm.com
Subject: Re: [cfe-dev] OpenCL & SPIR specific types - proposal and patch
Richard,
Thanks for your very helpful review.
Attached an updated patch – please review.
Thanks
From: metafoo at gmail.com [mailto:metafoo at gmail.com] On Behalf Of Richard
Smith
Sent: Sunday, October 07, 2012 20:35
To: Benyei, Guy
Cc: Villmow, Micah; Tanya Lattner; Anton.Lokhmotov at arm.com;
cfe-dev at cs.uiuc.edu
Subject: Re: [cfe-dev] OpenCL & SPIR specific types - proposal and patch
+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<BuiltinType>(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;
+
+ switch (cast<BuiltinType>(T)->getKind()) {
+ default: break;
+ case BuiltinType::Image1d_T:
+ ResultType = llvm::PointerType::get(llvm::StructType::create(
+ CGM.getLLVMContext(),"opencl.image1d_t"), 0);
+ break;
In your 'default' case, you return an uninitialized value. Please change
this to return the PointerType directly, rather than pushing it through a
local variable, and add an llvm_unreachable to your default case.
+ friend class CodeGenTypes;
[...]
+ ResultType = CGM.OpenCLRuntime->convertOpenCLSpecificType(Ty);
Please remove the 'friend' declaration and instead use
CGM.getOpenCLRuntime() here.
Please also add tests for the PCH serialization/deserialization of these
types.
On Sun, Oct 7, 2012 at 2:07 AM, Benyei, Guy <guy.benyei at intel.com> wrote:
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
From: Villmow, Micah [mailto:Micah.Villmow at amd.com]
Sent: Thursday, October 04, 2012 22:15
To: Tanya Lattner; Benyei, Guy
Cc: cfe-dev at cs.uiuc.edu; Anton.Lokhmotov at arm.com
Subject: RE: [cfe-dev] OpenCL & SPIR specific types - proposal and patch
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
From: Tanya Lattner [mailto:lattner at apple.com]
Sent: Thursday, October 04, 2012 11:49 AM
To: Benyei, Guy
Cc: cfe-dev at cs.uiuc.edu; Villmow, Micah; Anton.Lokhmotov at arm.com
Subject: Re: [cfe-dev] OpenCL & SPIR specific types - proposal and patch
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
On Oct 3, 2012, at 8:06 AM, "Benyei, Guy" <guy.benyei at intel.com> wrote:
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>
---------------------------------------------------------------------
Intel Israel (74) Limited
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
cfe-dev mailing list
cfe-dev at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
---------------------------------------------------------------------
Intel Israel (74) Limited
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
cfe-dev mailing list
cfe-dev at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
---------------------------------------------------------------------
Intel Israel (74) Limited
This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
More information about the cfe-dev
mailing list