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
