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

[Benyei, Guy]

Hello,

+    LongLongWidth = 64; LongLongAlign = 64;

The "long long" type is currently reserved for future use in OpenCL. I don't think forcing it to be 64 bit is right.

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

The idea behind adding the AddrSpaceMap was to support different ordering of the address spaces. AFAIK, the different vendors using Clang as frontend compiler to OpenCL currently use different ordering. The current SPIR spec also defines a different order (3-local, 2-constant), and the CUDA address spaces are missing. Anyhow, I don't think any numbering should be forced here.

+int v4[(sizeof(long long) == 8) -1];
+int v5[(__alignof(long long) == 8) -1];

Again, "long long" is reserved.

Thanks
    Guy Benyei


-----Original Message-----
From: cfe-commits-bounces at cs.uiuc.edu [mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of Richard Smith
Sent: Saturday, December 08, 2012 00:19
To: David Tweed
Cc: cfe-commits at cs.uiuc.edu; tonic at nondot.org
Subject: Re: [cfe-commits] [PATCH v3] Set some OpenCl specification mandated types/alignments/etc

+    //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
>

_______________________________________________
cfe-commits mailing list
cfe-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.