Ping. Any further comments on this one? On Wed, 24 Jun 2015 at 10:43 Pedro Ferreira <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote: >> >>> On Thu, Jun 18, 2015 at 5:41 PM, Eric Christopher <[email protected]> >>> 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 <[email protected]> >>>> wrote: >>>> >>>>> Updated from head SVN - no conflicts. >>>>> Still runs without failures. >>>>> >>>>> >>>>> On Wed, 17 Jun 2015 at 15:08 Pedro Ferreira <[email protected]> >>>>> 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 < >>>>>> [email protected]> 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:[email protected]] >>>>>>> *Sent:* 15 June 2015 09:16 >>>>>>> *To:* Eric Christopher; Anastasia Stulova; [email protected] >>>>>>> >>>>>>> >>>>>>> *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 <[email protected]> >>>>>>> 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 <[email protected]> >>>>>>> wrote: >>>>>>> >>>>>>> Awesome, thanks for the tips. >>>>>>> Updated version attached. >>>>>>> >>>>>>> >>>>>>> >>>>>>> Pedro >>>>>>> >>>>>>> >>>>>>> >>>>>>> On Thu, 11 Jun 2015 at 19:23 Anastasia Stulova < >>>>>>> [email protected]> 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 [[email protected]] >>>>>>> Sent: Thursday, June 11, 2015 12:50 PM >>>>>>> To: Anastasia Stulova; [email protected] >>>>>>> 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 <[email protected] >>>>>>> <mailto:[email protected]>> 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 <[email protected] >>>>>>> <mailto:[email protected]>> 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 < >>>>>>> [email protected]<mailto:[email protected]>> 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: [email protected]<mailto: >>>>>>> [email protected]> [[email protected] >>>>>>> <mailto:[email protected]>] On Behalf Of Pedro >>>>>>> Ferreira [[email protected]<mailto:[email protected]>] >>>>>>> Sent: Thursday, June 11, 2015 8:18 AM >>>>>>> To: [email protected]<mailto:[email protected]> >>>>>>> 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 >>>>>>> [email protected] >>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>>>> >>>>>>> >>>> _______________________________________________ >>>> cfe-commits mailing list >>>> [email protected] >>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>> >>>>
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
