[PATCH] OpenCL: Add new types for OpenCL 2.0

Richard Smith richard at metafoo.co.uk
Thu Jun 18 20:32:30 PDT 2015 

On Thu, Jun 18, 2015 at 5:41 PM, Eric Christopher <echristo at gmail.com>
wrote:

> Please run clang-format on your patch. You still have lines over
> 80-columns for example.
>
+  bool isImage2dDepthT() const;                 // Opencl image_2d_depth_t
+  bool isImage2dArrayDepthT() const;            // Opencl
image_2d_array_depth_t
+  bool isImage2dMSAAT() const;                  // Opencl image_2d_msaa_t
+  bool isImage2dArrayMSAAT() const;             // Opencl
image_2d_array_msaa_t
+  bool isImage2dMSAATDepth() const;             // Opencl
image_2d_msaa_depth_t
+  bool isImage2dArrayMSAATDepth() const;        // Opencl
image_2d_array_msaa_depth_t

Here's an example of an over-80-columns line. Please also fix the
capitalization to "OpenCL" here to match the surrounding code.

> Thanks.
>
> -eric
>
> On Wed, Jun 17, 2015, 7:38 AM Pedro Ferreira <arkangath at gmail.com> wrote:
>
>> Updated from head SVN - no conflicts.
>> Still runs without failures.
>>
>>
>> On Wed, 17 Jun 2015 at 15:08 Pedro Ferreira <arkangath at gmail.com> wrote:
>>
>>> Sorry, that was my bad. I forgot to set my editor back to llvm
>>> indentation settings and it inserted tabs instead of spaces.
>>>
>>> Line 1000 of SemaTypes is 79 characters long, which is the largest
>>> (longest) line in the patch. It is the same length of line 981 from where I
>>> copy-pasted the code (by your suggestion).
>>>
>>> Grep claims the attached patch has no tabs now :)
>>>
>>> On Wed, 17 Jun 2015 at 14:47 Anastasia Stulova <
>>> anastasia.stulova at arm.com> wrote:
>>>
>>>> I think there are still lines that are too long (especially in
>>>> SemaType.cpp). Have you run clang-format on your changes?
>>>>
>>>>
>>>>
>>>> Otherwise, no further comments from my side.
>>>>
>>>>
>>>>
>>>> Anastasia
>>>>
>>>>
>>>>
>>>> *From:* Pedro Ferreira [mailto:arkangath at gmail.com]
>>>> *Sent:* 15 June 2015 09:16
>>>> *To:* Eric Christopher; Anastasia Stulova; cfe-commits at cs.uiuc.edu
>>>>
>>>>
>>>> *Subject:* Re: [PATCH] OpenCL: Add new types for OpenCL 2.0
>>>>
>>>>
>>>>
>>>> There were a couple lines with > 80 columns, and this new patch version
>>>> fixes them.
>>>>
>>>>
>>>>
>>>>
>>>>
>>>> On Fri, 12 Jun 2015 at 21:03 Eric Christopher <echristo at gmail.com>
>>>> wrote:
>>>>
>>>> Drive by review here.
>>>>
>>>>
>>>>
>>>> I was making sure there was debug info support for the bits, thanks for
>>>> adding it though I'm not seeing any tests ;)
>>>>
>>>>
>>>>
>>>> I'm pretty sure you have some 80-column violations and other formatting
>>>> things, could you clang-format your patch?
>>>>
>>>>
>>>>
>>>> Thanks!
>>>>
>>>>
>>>>
>>>> -eric
>>>>
>>>> On Fri, Jun 12, 2015 at 4:20 AM Pedro Ferreira <arkangath at gmail.com>
>>>> wrote:
>>>>
>>>> Awesome, thanks for the tips.
>>>> Updated version attached.
>>>>
>>>>
>>>>
>>>> Pedro
>>>>
>>>>
>>>>
>>>> On Thu, 11 Jun 2015 at 19:23 Anastasia Stulova <
>>>> Anastasia.Stulova at arm.com> wrote:
>>>>
>>>> CodeGen tests looks good!
>>>>
>>>> Regarding the extension, could you diagnose it during the type checking
>>>> instead. That way it will be cover all cases. You can look at the CL2.0
>>>> atomic type implementation in SemaType.cpp ConvertDeclSpecToType. Also
>>>> please reuse the same error err_type_requires_extension instead of adding
>>>> the new one. Please, add Sema test demonstating the error handling works
>>>> correctly.
>>>>
>>>> Thanks,
>>>> Anastasia
>>>> ________________________________________
>>>> From: Pedro Ferreira [arkangath at gmail.com]
>>>> Sent: Thursday, June 11, 2015 12:50 PM
>>>> To: Anastasia Stulova; cfe-commits at cs.uiuc.edu
>>>> Subject: Re: [PATCH] OpenCL: Add new types for OpenCL 2.0
>>>>
>>>> Ok, found out the right place to diagnose the extension and added the
>>>> tests.
>>>> I am not particularly convinced that was the best way to do it;
>>>> comments welcome.
>>>>
>>>> Pedro
>>>>
>>>> On Thu, 11 Jun 2015 at 11:43 Pedro Ferreira <arkangath at gmail.com
>>>> <mailto:arkangath at gmail.com>> wrote:
>>>> Actually, I spoke too soon - I found a test with -cl-std=CL2.0. I
>>>> missed that.
>>>>
>>>> On Thu, 11 Jun 2015 at 11:40 Pedro Ferreira <arkangath at gmail.com
>>>> <mailto:arkangath at gmail.com>> wrote:
>>>> The codegen test would imply adding a -cl-std=2.0 option to Clang,
>>>> which it currently does not have. This is because the types should only be
>>>> recognised if the CL 2.0 standard is explicitly asked for (the default is
>>>> to operate on 1.2 mode). Adding that option is a peripheral issue. I've
>>>> added the types on the header test under the appropriate "#if defined" but
>>>> when I tried to do the same on the .cl file, I found out that the test
>>>> parser does not recognise the preprocessor macro and therefore was causing
>>>> the test to (incorrectly) fail. As such, I reverted the test.
>>>>
>>>> As for the AS for the other types, I copy-pasted the code from event_t.
>>>> That's the reason why I'm actually using the "0". Are you suggesting I
>>>> should change event_t to use something else, and by consequence the new
>>>> types too? That would be a separate issue.
>>>> My guess is that these types are allocated on the stack, which by llvm
>>>> convention will always be 0.
>>>>
>>>> The new types are used by new builtins. I don't think there are any
>>>> other special semantics to it.
>>>>
>>>> I've added extension checks on the MSAA types, but I'm not sure if this
>>>> is the right place. New patch attached.
>>>>
>>>> Pedro
>>>>
>>>> On Thu, 11 Jun 2015 at 10:33 Anastasia Stulova <
>>>> Anastasia.Stulova at arm.com<mailto:Anastasia.Stulova at arm.com>> wrote:
>>>> Hi Pedro,
>>>>
>>>> Could we also add a Codegen test? Also it would be better not to use
>>>> constant directly as address space as the mapping could ideally be changed.
>>>> Is there any reason why you generate pointers to private AS?
>>>>
>>>> Are there any operations allowed on new types? Any semantical checks
>>>> needed?
>>>>
>>>> If MSAA types are part of an extension and not a part of the general
>>>> standard we should ideally diagnose that extension is enabled when they are
>>>> being used.
>>>>
>>>> Regards,
>>>> Anastasia
>>>> ________________________________________
>>>> From: cfe-commits-bounces at cs.uiuc.edu<mailto:
>>>> cfe-commits-bounces at cs.uiuc.edu> [cfe-commits-bounces at cs.uiuc.edu
>>>> <mailto:cfe-commits-bounces at cs.uiuc.edu>] On Behalf Of Pedro Ferreira [
>>>> arkangath at gmail.com<mailto:arkangath at gmail.com>]
>>>> Sent: Thursday, June 11, 2015 8:18 AM
>>>> To: cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
>>>> Subject: [PATCH] OpenCL: Add new types for OpenCL 2.0
>>>>
>>>> Hi all,
>>>>
>>>> This patch adds the new OpenCL types for 2.0 described at
>>>> https://www.khronos.org/registry/cl/sdk/2.0/docs/man/xhtml/otherDataTypes.html
>>>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__www.khronos.org_registry_cl_sdk_2.0_docs_man_xhtml_otherDataTypes.html&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=9Bdb39VF2l8sby_fS5dvbUnJSVMkbUEkua5v-UqAuGY&s=5Dqa4a6V-GRZkKn3l59ia5wJtJJzBqEjUrQlOV-8t-w&e=>
>>>> <
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.khronos.org_registry_cl_sdk_2.0_docs_man_xhtml_otherDataTypes.html&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=42YnWExwxwpeU6GPDY2_3RFxCqQakUbj_CXZsMsQ2jU&s=REOBNoaDio7qDyIDCqmXhxFvZYjMOK6vuXAttjOVsNI&e=
>>>> >
>>>> I also opened https://llvm.org/bugs/show_bug.cgi?id=23794
>>>> <https://urldefense.proofpoint.com/v2/url?u=https-3A__llvm.org_bugs_show-5Fbug.cgi-3Fid-3D23794&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=9Bdb39VF2l8sby_fS5dvbUnJSVMkbUEkua5v-UqAuGY&s=DaxUQk4vxUwKYs93ZTAVu1S6Hdg2CQ1J96KmGJWtfDg&e=>
>>>> <
>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__llvm.org_bugs_show-5Fbug.cgi-3Fid-3D23794&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=BSqEv9KvKMW_Ob8SyngJ70KdZISM_ASROnREeq0cCxk&m=42YnWExwxwpeU6GPDY2_3RFxCqQakUbj_CXZsMsQ2jU&s=TAV4suAMaHgdIPA83Da3pQl7c68On7bAFWtnrUbt_Uk&e=>
>>>> for this. I keep forgetting you prefer patches sent to this mailing list.
>>>> This also adds lldb entries (fixes switch warnings).
>>>>
>>>> The types are:
>>>>
>>>> image2d_depth_t
>>>> image2d_array_depth_t
>>>> image2d_msaa_t
>>>> image2d_array_msaa_t
>>>> image2d_msaa_depth_t
>>>> image2d_array_msaa_depth_t
>>>> queue_t
>>>> ndrange_t
>>>> clk_event_t
>>>> reserve_id_t
>>>>
>>>> let me know if something looks wrong,
>>>> Pedro
>>>>
>>>> -- IMPORTANT NOTICE: The contents of this email and any attachments are
>>>> confidential and may also be privileged. If you are not the intended
>>>> recipient, please notify the sender immediately and do not disclose the
>>>> contents to any other person, use it for any purpose, or store or copy the
>>>> information in any medium.  Thank you.
>>>>
>>>> ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ,
>>>> Registered in England & Wales, Company No:  2557590
>>>> ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1
>>>> 9NJ, Registered in England & Wales, Company No:  2548782
>>>>
>>>>
>>>> -- IMPORTANT NOTICE: The contents of this email and any attachments are
>>>> confidential and may also be privileged. If you are not the intended
>>>> recipient, please notify the sender immediately and do not disclose the
>>>> contents to any other person, use it for any purpose, or store or copy the
>>>> information in any medium.  Thank you.
>>>>
>>>> ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ,
>>>> Registered in England & Wales, Company No:  2557590
>>>> ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1
>>>> 9NJ, Registered in England & Wales, Company No:  2548782
>>>>
>>>> _______________________________________________
>>>> cfe-commits mailing list
>>>> cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>
>>>>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150618/a3399102/attachment.html>

More information about the cfe-commits mailing list