Hi Dave,
+ static const LangAS::Map OpenCLAddrSpaceMap = {
+ 0, // opencl_private
+ 1, // opencl_global
+ 2, // opencl_constant
+ 3 // opencl_local
+ };
The issue here is that the order in this mape should be different. The order is
defined by the following enum in AddressSpaces.h:
enum ID {
Offset = 0xFFFF00,
opencl_global = Offset,
opencl_local,
opencl_constant,
cuda_device,
cuda_constant,
cuda_shared,
Last,
Count = Last-Offset
};
This eumeration is used for the IR generation, so the order is local and then
constant, unlike in your comments. Then again, SPIR doesn't mandate the
different vendors to use a single numbering for their own IR.
Thanks
Guy
-----Original Message-----
From: David Tweed [mailto:[email protected]]
Sent: Tuesday, December 11, 2012 17:22
To: Benyei, Guy; Richard Smith
Cc: [email protected]; [email protected]
Subject: RE: [cfe-commits] [PATCH v3] Set some OpenCl specification mandated
types/alignments/etc
Thanks for the comments. I'll consult about whether we still think this is the
right way to do things; it certainly sounds like the address space is
sufficiently variable that it shouldn't go into setForcedLangOptions even if
the more fixed things are. The "long long" size was indeed a typo. As regards
address space qualifier numbering, the current SPIR draft on
www.khronos.org/registry/cl still specifies this mapping in sec 2.2 on page 14.
Regards,
Dave
-----Original Message-----
From: Benyei, Guy [mailto:[email protected]]
Sent: 09 December 2012 07:25
To: Richard Smith; David Tweed
Cc: [email protected]; [email protected]
Subject: RE: [cfe-commits] [PATCH v3] Set some OpenCl specification mandated
types/alignments/etc
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: [email protected]
[mailto:[email protected]] On Behalf Of Richard Smith
Sent: Saturday, December 08, 2012 00:19
To: David Tweed
Cc: [email protected]; [email protected]
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 <[email protected]> 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:[email protected]]
> Sent: 30 November 2012 10:23
> To: David Tweed; David Tweed; 'Eli Friedman'
> Cc: 'Pekka Jääskeläinen'; '[email protected]'
> 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:[email protected]]
> Sent: 27 November 2012 16:11
> To: David Tweed; 'Eli Friedman'
> Cc: 'Pekka Jääskeläinen'; '[email protected]'
> Subject: RE: [PATCH v2] Set some OpenCl specification mandated
> types/alignments/etc
>
> Revised patch actually attached.
>
> -----Original Message-----
> From: David Tweed [mailto:[email protected]]
> Sent: 27 November 2012 16:09
> To: 'Eli Friedman'
> Cc: Pekka Jääskeläinen; [email protected]
> 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
> [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
---------------------------------------------------------------------
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.
---------------------------------------------------------------------
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.
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
