[cfe-dev] [OpenCL patch] asType, Convert **revised**

[Tanya Lattner]

On May 26, 2011, at 7:55 AM, Anton Lokhmotov wrote:

> 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

> 
> Best regards,
> Anton.
> 
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110526/45c40b39/attachment.html>