Re: [cfe-commits] [PATCH v2] Set some OpenCl specification mandated types/alignments/etc
ping for comments/objections/exasperation on this general approach? Regards, Dave -Original Message- From: David Tweed [mailto:david.tw...@arm.com] Sent: 27 November 2012 16:11 To: David Tweed; 'Eli Friedman' Cc: 'Pekka Jääskeläinen'; 'cfe-commits@cs.uiuc.edu' Subject: RE: [PATCH v2] Set some OpenCl specification mandated types/alignments/etc Revised patch actually attached. -Original Message- From: David Tweed [mailto:david.tw...@arm.com] Sent: 27 November 2012 16:09 To: 'Eli Friedman' Cc: Pekka Jääskeläinen; cfe-commits@cs.uiuc.edu Subject: RE: [PATCH v2] Set some OpenCl specification mandated types/alignments/etc Hi Eli, thanks for the feedback. In general I've tested everything I know how to test using only the output of a compilation. | Why are you messing with LargeArrayMinWidth and LargeArrayAlign? | Please explain in the patch and add tests. That looks like something I forgot was an implementation thing rather than standard. Removed. | Why are you messing with the floating point formats (particularly | without setting the size and alignment alongside the format)? Please | explain in the patch and add tests. I didn't really think about the floating point formats possibly changing size; fixed. Explained in patch that all these things are language specified. | Is a fixed OpenCL address map really appropriate for every target? This patch sets up the mandated address spaces for OpenCL; presumably targets that want to add more can extend it themselves. | Also, please wait at least a couple more days; all the Apple people | were off last week, and I want to give them time to comment. No problem waiting a while: the problem with patches areas (like this one) in extremely dull areas is knowing if there's no response just because it's so uninteresting rather than there hasn't been time for commenters, but I forgot to factor in American Thanksgiving. Regards, Dave ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [cfe-commits] [PATCH v2] Set some OpenCl specification mandated types/alignments/etc
Revised patch actually attached. -Original Message- From: David Tweed [mailto:david.tw...@arm.com] Sent: 27 November 2012 16:09 To: 'Eli Friedman' Cc: Pekka Jääskeläinen; cfe-commits@cs.uiuc.edu Subject: RE: [PATCH v2] Set some OpenCl specification mandated types/alignments/etc Hi Eli, thanks for the feedback. In general I've tested everything I know how to test using only the output of a compilation. | Why are you messing with LargeArrayMinWidth and LargeArrayAlign? | Please explain in the patch and add tests. That looks like something I forgot was an implementation thing rather than standard. Removed. | Why are you messing with the floating point formats (particularly | without setting the size and alignment alongside the format)? Please | explain in the patch and add tests. I didn't really think about the floating point formats possibly changing size; fixed. Explained in patch that all these things are language specified. | Is a fixed OpenCL address map really appropriate for every target? This patch sets up the mandated address spaces for OpenCL; presumably targets that want to add more can extend it themselves. | Also, please wait at least a couple more days; all the Apple people | were off last week, and I want to give them time to comment. No problem waiting a while: the problem with patches areas (like this one) in extremely dull areas is knowing if there's no response just because it's so uninteresting rather than there hasn't been time for commenters, but I forgot to factor in American Thanksgiving. Regards, Dave opencl3.diff Description: Binary data ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [cfe-commits] [PATCH v2] Set some OpenCl specification mandated types/alignments/etc
Hi Eli, thanks for the feedback. In general I've tested everything I know how to test using only the output of a compilation. | Why are you messing with LargeArrayMinWidth and LargeArrayAlign? | Please explain in the patch and add tests. That looks like something I forgot was an implementation thing rather than standard. Removed. | Why are you messing with the floating point formats (particularly | without setting the size and alignment alongside the format)? Please | explain in the patch and add tests. I didn't really think about the floating point formats possibly changing size; fixed. Explained in patch that all these things are language specified. | Is a fixed OpenCL address map really appropriate for every target? This patch sets up the mandated address spaces for OpenCL; presumably targets that want to add more can extend it themselves. | Also, please wait at least a couple more days; all the Apple people | were off last week, and I want to give them time to comment. No problem waiting a while: the problem with patches areas (like this one) in extremely dull areas is knowing if there's no response just because it's so uninteresting rather than there hasn't been time for commenters, but I forgot to factor in American Thanksgiving. Regards, Dave ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
[cfe-commits] [PATCH v2] Set some OpenCl specification mandated types/alignments/etc
Following the comments, it looks like there's not a strong objection to doing OpenCL mandated configuration this way and some support for this. So the attached patch is modified as suggested to use a specific target when testing the sizes/alignments. Unless there are further objections I'll commit in a day or so. Regards, Dave -Original Message- From: cfe-commits-boun...@cs.uiuc.edu [mailto:cfe-commits-boun...@cs.uiuc.edu] On Behalf Of Pekka Jääskeläinen Sent: 23 November 2012 09:31 To: cfe-commits@cs.uiuc.edu Subject: Re: [cfe-commits] [PATCH] Set some OpenCl specification mandated types/alignments/etc Hi, If I understood correctly, this patch addresses the issue I asked in the lists some time ago (but failed to provide a patch due to no time). http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-January/019528.html So, if it does, definitely a +1 from me, someone who's trying to produce a widely portable OpenCL implementation on top of Clang/LLVM. http://pocl.sourceforge.net It should make it easier to support 32-bit targets: https://bugs.launchpad.net/pocl/+bug/911911 Thanks! On 11/23/2012 11:11 AM, David Tweed wrote: Just to keep this on the radar: is there anybody, particularly anyone implementing OpenCL in some way, who thinks that specification mandated front-end stuff _definitely shouldn't_ be defined via setForcedLangOptions (with per-target/implementation stuff set elsewhere)? (Stating the obvious, it's desirable to keep as much really standard OpenCL stuff within the mainstream clang front-end rather than in implementor specific patches.) Cheers, Dave -Original Message- From: cfe-commits-boun...@cs.uiuc.edu [mailto:cfe-commits-boun...@cs.uiuc.edu] On Behalf Of David Tweed Sent: 22 November 2012 08:50 To: Eli Friedman Cc: llvm cfe Subject: Re: [cfe-commits] [PATCH] Set some OpenCl specification mandated types/alignments/etc It's difficult to tell what the best split up is: the elements in the patch are things that can't, by spec, be defined any other way by alternative implementations. So is it more maintainable to have them in a block and have target/implementation specific tweaks elsewhere, or move everything to be per-target? My mild preference is still for the former, but if the consensus from others is on the later I'll try to rework things. I'll have a look at putting a specific triple on the tests. Thanks, -Original Message- From: Eli Friedman [mailto:eli.fried...@gmail.com] Sent: 21 November 2012 23:42 To: David Tweed Cc: llvm cfe Subject: Re: [cfe-commits] [PATCH] Set some OpenCl specification mandated types/alignments/etc On Wed, Nov 21, 2012 at 3:52 AM, David Tweed david.tw...@arm.com wrote: Hi, the attached patch sets uses setForcedLangOptions to set certain types/alignments/etc which are completely specified by the OpenCL or SPIR specs. There's also a test for those things directly exposed at the user level. (The test is in Misc because the only specific OpenCL directory is for code-gen, which this isn't really... but I can move it somewhere else if desired.) Please review and if ok I'll commit. Here's a potential counter-proposal: add new targets for OpenCL, because there could potentially be other things an OpenCL implementation needs to tweak on a per-target basis (e.g. #defines). I would like to see a comment from someone else with a different OpenCL implementation to check what vendors are currently doing. If you think this really is the best approach, please change the test to specifically test triples you consider important. -Eli ___ 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 -- --Pekka ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits opencl2.diff Description: Binary data ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
Re: [cfe-commits] [PATCH v2] Set some OpenCl specification mandated types/alignments/etc
On Mon, Nov 26, 2012 at 9:05 AM, David Tweed david.tw...@arm.com wrote: Following the comments, it looks like there's not a strong objection to doing OpenCL mandated configuration this way and some support for this. So the attached patch is modified as suggested to use a specific target when testing the sizes/alignments. Unless there are further objections I'll commit in a day or so. More comments from another look at the patch: Why are you messing with LargeArrayMinWidth and LargeArrayAlign? Please explain in the patch and add tests. Why are you messing with the floating point formats (particularly without setting the size and alignment alongside the format)? Please explain in the patch and add tests. Is a fixed OpenCL address map really appropriate for every target? Also, please wait at least a couple more days; all the Apple people were off last week, and I want to give them time to comment. -Eli ___ cfe-commits mailing list cfe-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
