On Thu, Jun 18, 2015 at 5:41 PM, Eric Christopher <echri...@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 <arkang...@gmail.com> wrote: > >> Updated from head SVN - no conflicts. >> Still runs without failures. >> >> >> On Wed, 17 Jun 2015 at 15:08 Pedro Ferreira <arkang...@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.stul...@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:arkang...@gmail.com] >>>> *Sent:* 15 June 2015 09:16 >>>> *To:* Eric Christopher; Anastasia Stulova; cfe-commits@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 <echri...@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 <arkang...@gmail.com> >>>> wrote: >>>> >>>> Awesome, thanks for the tips. >>>> Updated version attached. >>>> >>>> >>>> >>>> Pedro >>>> >>>> >>>> >>>> On Thu, 11 Jun 2015 at 19:23 Anastasia Stulova < >>>> anastasia.stul...@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 [arkang...@gmail.com] >>>> Sent: Thursday, June 11, 2015 12:50 PM >>>> To: Anastasia Stulova; cfe-commits@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 <arkang...@gmail.com >>>> <mailto:arkang...@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 <arkang...@gmail.com >>>> <mailto:arkang...@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.stul...@arm.com<mailto:anastasia.stul...@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-boun...@cs.uiuc.edu<mailto: >>>> cfe-commits-boun...@cs.uiuc.edu> [cfe-commits-boun...@cs.uiuc.edu >>>> <mailto:cfe-commits-boun...@cs.uiuc.edu>] On Behalf Of Pedro Ferreira [ >>>> arkang...@gmail.com<mailto:arkang...@gmail.com>] >>>> Sent: Thursday, June 11, 2015 8:18 AM >>>> To: cfe-commits@cs.uiuc.edu<mailto:cfe-commits@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@cs.uiuc.edu >>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>> >>>> > _______________________________________________ > cfe-commits mailing list > cfe-commits@cs.uiuc.edu > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >
_______________________________________________ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
