[OpenCL patch] asType, Convert **revised**

Hi Tanya,

As I indicated 2 months ago, we are not in favour of this approach:
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-March/014078.html

I was under the impression that neither was Chris:
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-March/014123.html
nor was Guy:
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2011-March/013692.html

I think more discussion is needed if this is to go in.

Fair enough, but as you can understand its difficult to have patches live outside the tree for so long.

I disagree that Chris was against the approach entirely, but I do know that we have differing opinions.

I do believe that asType is the right approach and should be a builtin. I do not think it can be expressed in C to do a bitcast or a shuffle/bitcast. So having a built-in to do this, is the right way to go. If you have another implementation of asType, mine should not conflict with yours if you are using library functions.

As I have mentioned before, AsType as an ExprClass also has the added benefit as being part of the AST for the various tools that may want to take advantage of this (ie. syntax highlighting, etc).

My main concern is that ‘as_type’ and ‘convert_type’ [not ‘astype’ and
‘convert’] are not that different from other overloaded OpenCL built-in
functions. While ‘as_type’ can indeed be represented in LLVM-IR as a
bitcast or shuffle, different flavours of ‘convert_type’ are effectively
built-in functions, to be implemented either in the library or in the
backend, at which level I’m not sure what e.g. __builtin_convert(i, short,
2, 1) buys you over convert_short_rtz_sat(i).

As I understand, this works for you as follows (please correct me if I’m
wrong).

OpenCL ‘convert_type’ functions are #define’d in your internal header file
as e.g.:
#define convert_short_rtz_sat(i) __builtin_convert(i, short, 2, 1)

Parsing a __builtin_convert by Sema::ActOnConvertExpr results in a
ConvertExpr expression.

This expression is then visited by ScalarExprEmitter::VisitConvertExpr,
which creates a call to one of the llvm::Intrinsic::convert?? intrinsics
with { Src, Mode, Sat } parameters [ Mode → RoundingMode; Sat →
SaturatedMode ].

So why is this better than just placing declarations for the ‘convert_type’
functions in the same header file as for all other built-in functions?

I know understand your concerns about convert, and I will remove it from this patch at this time. We will investigate an alternative approach.

+KEYWORD(__builtin_astype , KEYOPENCL)

+KEYWORD(__builtin_convert , KEYOPENCL)

Please note that these are not keywords according to the OpenCL
specification. [astype → as_type; convert → convert_type]

While not a keyword, it is for OpenCL only. I’m happy to change it back, but I think it probably should stay this way.

I will re-do the patch to only handle __builtin_astype.

-Tanya

Hi Tanya,

Thank you for splitting from this patch. I agree it's fine to have as_type
in the AST. Minor comments below.

Best, Anton.

+// OpenCL warnings and errors.
+def err_invalid_astype_of_different_size : Error<
+ "invalid astype between type '%0' and '%1' of different size">;