[cfe-dev] [OpenCL patch] support for __builtin_astype, __builtin_convert, __builtin_vec_step

[Benyei, Guy]

Hi Tanya,
Thanks for the patch; it's great to get your contribution for the OpenCL support.

Creating operators that may get types as arguments for as_typen and convert is a very elegant solution; however it's quite intrusive, as these features might be implemented as standard functions, and linked to the user's code later. As_typen needs some special checks, so it could be a clang built-in, but conversions basically could be implemented without changing clang at all.

Vec_step is very similar to the sizeof and the alignof operators, so I think it's OK to implement it using the same expression type, instead of code duplication.


Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td                (revision 125808)
+++ include/clang/Basic/DiagnosticSemaKinds.td             (working copy)
@@ -3723,6 +3723,26 @@
   "%0 does not refer to the name of a parameter pack; did you mean %1?">;
def note_parameter_pack_here : Note<"parameter pack %0 declared here">;
...
+def err_cvt_arg_must_be_constant : Error<
+  "__builtin_convert requires constant 3rd and 4th argument">;
+def err_invalid_astype_of_different_size : Error<
+  "invalid astype between type '%0' and '%1' of different size">;
+def err_vec_step_bitfield : Error<
+  "invalid application of 'vec_step' to bitfield">;

Bitfields are not supported in OpenCL at all - is this error message really needed?

Index: include/clang/Basic/TokenKinds.def
===================================================================
--- include/clang/Basic/TokenKinds.def (revision 125808)
+++ include/clang/Basic/TokenKinds.def              (working copy)
@@ -356,6 +356,11 @@
KEYWORD(__vector                    , KEYALTIVEC)
KEYWORD(__pixel                     , KEYALTIVEC)

+// OpenCL Extensions.
+KEYWORD(__builtin_astype            , KEYALL)
+KEYWORD(__builtin_convert           , KEYALL)
+KEYWORD(__builtin_vec_step          , KEYALL)
+

The latest version of CLANG already has KEYOPENCL flag, so keywords can be flagged as OpenCL only. This way vec_step could be a keyword, instead of using macros later to turn it to __builtin_vec_step. Also, it saves later checking of right target language.

Index: lib/CodeGen/CGExprScalar.cpp
===================================================================
--- lib/CodeGen/CGExprScalar.cpp           (revision 125808)
+++ lib/CodeGen/CGExprScalar.cpp        (working copy)
@@ -2534,6 +2537,86 @@
   return CGF.EmitBlockLiteral(block);
}

+Value *ScalarExprEmitter::VisitAsTypeExpr(AsTypeExpr *E) {
+  Value *Src  = CGF.EmitScalarExpr(E->getSrcExpr());
+  const llvm::Type * DstTy = ConvertType(E->getDstType());
+
+  // Going from vec4->vec3 or vec3->vec4 is a special case and requires
+  // a shuffle vector instead of a bitcast.
+  const llvm::Type *SrcTy = Src->getType();
+  if (isa<llvm::VectorType>(DstTy) && isa<llvm::VectorType>(SrcTy)) {
+    unsigned numElementsDst = cast<llvm::VectorType>(DstTy)->getNumElements();
+    unsigned numElementsSrc = cast<llvm::VectorType>(SrcTy)->getNumElements();
+
+    if ((numElementsDst == 3 && numElementsSrc == 4)
+        || (numElementsDst == 4 && numElementsSrc == 3)) {

The OpenCL spec defines the behavior of vec4->vec3, but about vec3->vec4 it writes:
  float3 f;
  // Error. Result and operand have different sizes
  float4 g = as_float4(f);

Also, shuffle vector is not enough - there might be a conversion from int4 to float3, which is legal, but will crash the compiler if it's done only by shuffle. There must be a bitcast here.

+  if (dst_fp && src_fp)
+    ID = llvm::Intrinsic::convertff;
+  else if (dst_fp && !src_fp)
+    ID = src_s ? llvm::Intrinsic::convertfsi : llvm::Intrinsic::convertfui;
+  else if (!dst_fp && src_fp)
+    ID = dst_s ? llvm::Intrinsic::convertsif : llvm::Intrinsic::convertuif;
+  else if (dst_s && src_s)
+    ID = llvm::Intrinsic::convertss;
+  else if (dst_s && !src_s)
+    ID = llvm::Intrinsic::convertsu;
+  else if (!dst_s && src_s)
+    ID = llvm::Intrinsic::convertus;
+  else
+    ID = llvm::Intrinsic::convertuu;
+

These intrinsics are not documented. Are they safe to use? Do they fulfill the OpenCL spec's precision requirements?

Index: lib/Parse/ParseExpr.cpp
===================================================================
--- lib/Parse/ParseExpr.cpp         (revision 125808)
+++ lib/Parse/ParseExpr.cpp      (working copy)
...
+  // We know that it is either a parenthesized type-name, or it is a
+  // unary-expression that starts with a compound literal, or starts with
+  // a primary-expression that is a parenthesized expression.
+  ParenParseOption ExprType = CastExpr;
+  ParsedType CastTy;
+  SourceLocation LParenLoc = Tok.getLocation(), RParenLoc;
+
+  // C++0x [expr.sizeof]p1:
+  //   [...] The operand is either an expression, which is an unevaluated
+  //   operand (Clause 5) [...]
+  //
+  // The GNU typeof and alignof extensions also behave as unevaluated
+  // operands.

Sizeof, alignof and typeof are not relevant here - I think vec_step should use the same infrastructure as sizeof, instead of code duplication.


Thanks
      Guy Benyei
      Intel
      SSG - MGP OpenCL Development Center



[cid:image001.png at 01CBD204.C07EF900]

---------------------------------------------------------------------
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110221/edb65504/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image001.png
Type: image/png
Size: 24800 bytes
Desc: image001.png
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20110221/edb65504/attachment.png>