[PATCH] OpenCL: Add new types for OpenCL 2.0

Pedro Ferreira arkangath at gmail.com
Thu Jul 2 08:31:40 PDT 2015


Thanks - I'll get to that once the Clang one is reviewed/approved.

On Thu, 2 Jul 2015 at 16:18 Eric Christopher <echristo at gmail.com> wrote:

> Ah. You should send it to the lldb list.
>
> On Thu, Jul 2, 2015, 12:56 AM Pedro Ferreira <arkangath at gmail.com> wrote:
>
>> The one I had attached on the very first mail :)
>> Re-attaching just in case it got lost.
>>
>> I was not able to test it - I meant only to add switch cases to silence
>> build warnings.
>>
>>
>> On Wed, 1 Jul 2015 at 22:57 Eric Christopher <echristo at gmail.com> wrote:
>>
>>> What lldb patch?
>>>
>>> -eric
>>>
>>> On Tue, Jun 30, 2015 at 3:07 AM Pedro Ferreira <arkangath at gmail.com>
>>> wrote:
>>>
>>>> Ping.
>>>> Any further comments on this one?
>>>>
>>>> On Wed, 24 Jun 2015 at 10:43 Pedro Ferreira <arkangath at gmail.com>
>>>> wrote:
>>>>
>>>>> Would help to actually attach the thing.
>>>>>
>>>>> Is the patch to lldb good? It's not really important for me - I just
>>>>> didn't want to add warnings to the lldb build.
>>>>>
>>>>> (this time adding cfe commits)
>>>>>
>>>>> On Fri, 19 Jun 2015 at 08:23 Pedro Ferreira <arkangath at gmail.com>
>>>>> wrote:
>>>>>
>>>>>> Attached the patch after "svn diff --diff-cmd=diff -x-U0 |
>>>>>> clang-format-diff.py -i"
>>>>>>
>>>>>> On the file you mentioned (Type.h), further up there was already a
>>>>>> line with over 80 characters - the limit was exceeded by the comment, so I
>>>>>> figured that comments were an exemption.
>>>>>> FWIW, the line is "bool isObjCIndependentClassType() const;      //
>>>>>> __attribute__((objc_independent_class))", which is 91 characters long.
>>>>>>
>>>>>> On Fri, 19 Jun 2015 at 04:32 Richard Smith <richard at metafoo.co.uk>
>>>>>> wrote:
>>>>>>
>>>>>>> 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
>>>>>>>>
>>>>>>>> _______________________________________________
>>>> 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/20150702/5f15b5b9/attachment.html>


More information about the cfe-commits mailing list