[cfe-commits] [PATCH v3] Set some OpenCl specification mandated types/alignments/etc

[Richard Smith]

+    //OpenCL fully specifies the size, alignment and representation
of floating types

Missing space between // and comment. (Also missing full stop.)

+    //endian-specific spaces can be added by individual backends as desired

Missing capitalization, space full stop.

+    static const LangAS::Map OpenCLAddrSpaceMap = {
+      0, // opencl_private
+      1, // opencl_global
+      2, // opencl_constant
+      3  // opencl_local
+    };

This does not match AddressSpaces.h. You don't seem to have any tests
for these values.

+++ test/Misc/languageOptsOpenCL.cl

This test would fit better in test/SemaOpenCL/. Please follow the
existing convention for naming test files: lowercase with hyphens to
separate words.

It would also be nice to have several RUN: lines with different
triples, to test that we get the same sizes no matter what target we
choose.

On Fri, Dec 7, 2012 at 7:15 AM, David Tweed <david.tweed at arm.com> wrote:
> (Randomly adding various people who've been unwise enough to have contribute
> to OpenCL/SPIR thread using their email address.)
>
> ping^2 on the general approach, particularly anyone who's involved in OpenCL
> implementation. (Current status is Eli Friedman would like comments on
> general approach from others, there's been one positive response from Pekka,
> but we're trying to ensure silence==acceptance rather than ==didn't see
> mail.)
>
> Regards,
> Dave
>
> -----Original Message-----
> From: David Tweed [mailto:david.tweed at arm.com]
> Sent: 30 November 2012 10:23
> To: David Tweed; David Tweed; 'Eli Friedman'
> Cc: 'Pekka Jääskeläinen'; 'cfe-commits at cs.uiuc.edu'
> Subject: RE: [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.tweed at arm.com]
> Sent: 27 November 2012 16:11
> To: David Tweed; 'Eli Friedman'
> Cc: 'Pekka Jääskeläinen'; 'cfe-commits at 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.tweed at arm.com]
> Sent: 27 November 2012 16:09
> To: 'Eli Friedman'
> Cc: Pekka Jääskeläinen; cfe-commits at 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 at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>