[cfe-dev] OpenCL & SPIR specific types - proposal and patch

Tue Oct 9 15:21:23 PDT 2012

Debug info looks better, thanks.

-eric

On Tue, Oct 9, 2012 at 1:49 PM, Benyei, Guy <guy.benyei at intel.com> wrote:

>  I’ve refactored the code to remove the repeated parts, and also fixed
> Anton Lokhmotov’s comments.****
>
> ** **
>
> Please review.****
>
> ** **
>
> Thanks****
>
> [image: email_signature_guy_new2]****
>
> ** **
>
> *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…****
>
>  ****
>
> [image: email_signature_guy_new2]****
>
>  ****
>
> *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****
>
> [image: email_signature_guy_new2]****
>
>  ****
>
> *From:* metafoo at gmail.com [mailto:metafoo at gmail.com <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 <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.****
>
> ** **
>
> ** **
>
> ---------------------------------------------------------------------
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20121009/7087ab42/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image001.png
Type: image/png
Size: 24800 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20121009/7087ab42/attachment.png>