[cfe-dev] [OpenCL patch] vector logical operations (not, and, or) ** revised **

[Peter Collingbourne]

Hi Tanya,

On Fri, Mar 11, 2011 at 04:59:30PM -0800, Tanya Lattner wrote:
> Attaching a revised version of this patch with a few fixes:

Comments below.

> +  const VectorType *VTy = lex->getType()->getAs<VectorType>();
> +  unsigned TypeSize = Context.getTypeSize(VTy->getElementType());
> +  if (TypeSize == Context.getTypeSize(Context.CharTy))
> +    return Context.getExtVectorType(Context.CharTy, VTy->getNumElements());
> +  else if (TypeSize == Context.getTypeSize(Context.ShortTy))
> +    return Context.getExtVectorType(Context.ShortTy, VTy->getNumElements());
> +  else if (TypeSize == Context.getTypeSize(Context.IntTy))
> +    return Context.getExtVectorType(Context.IntTy, VTy->getNumElements());
> +  else if (TypeSize == Context.getTypeSize(Context.Int128Ty))
> +    return Context.getExtVectorType(Context.Int128Ty, VTy->getNumElements());
> +  else if (TypeSize == Context.getTypeSize(Context.LongTy))
> +    return Context.getExtVectorType(Context.LongTy, VTy->getNumElements());
> + 
> +  assert(TypeSize == Context.getTypeSize(Context.LongLongTy) &&
> +         "Unhandled vector element size in vector compare");
> +  return Context.getExtVectorType(Context.LongLongTy, VTy->getNumElements());

Isn't this essentially a reimplementation of getSignedVariant?
We should be able to reuse it here.

> -    } else {
> +    } else if (const ExtVectorType *EVT = resultType->getAs<ExtVectorType>()) {
> +      // Handle vectors.
> +      // ExtVector LNot returns the signed version of the input type.
> +      QualType EltTy =  EVT->getElementType();
> +      EltTy = EltTy->getAs<BuiltinType>()->getSignedVariant(Context);

What happens if EltTy is a floating point type?

> +      resultType = Context.getExtVectorType(EltTy, EVT->getNumElements());
> +      break;
> +    } 
> +    else {

Style nitpick: should be '} else {'

>        return ExprError(Diag(OpLoc, diag::err_typecheck_unary_expr)
>          << resultType << Input->getSourceRange());
>      }
> Index: lib/AST/Type.cpp
> ===================================================================
> --- lib/AST/Type.cpp	(revision 127366)
> +++ lib/AST/Type.cpp	(working copy)
> @@ -36,6 +36,27 @@
>    return false;
>  }
>  
> +QualType BuiltinType::getSignedVariant(ASTContext& Ctx) const {

This function should be a member of Type so that it can be used on
integer and integer vector types.

> +  assert(isUnsignedInteger() 
> +         && "Only unsigned integers have an unsigned variant");

It should also be able to cope with signed integer types, in which case
it would just return the original type back.  The test to use here is
hasIntegerRepresentation(), which is true if this type has an integer
representation of some sort, e.g., it is an integer type or a vector.

> +  switch (BuiltinTypeBits.Kind) {
> +    case Bool: return Ctx.BoolTy;
> +    case Char_U: return Ctx.CharTy;

This case should use Ctx.SignedCharTy (char could be signed or
unsigned, and specifically if we see a Char_U then char is unsigned).

>  Value *ScalarExprEmitter::VisitBinLAnd(const BinaryOperator *E) {
> +  
> +  // If this is vector-typed, just emit two vcmp nodes and a vector and.
> +  if (E->getType()->isVectorType()) {
> +    Value *LHS = Visit(E->getLHS());
> +    Value *RHS = Visit(E->getRHS());
> +    Value *Zero = llvm::ConstantAggregateZero::get(LHS->getType());
> +    LHS = Builder.CreateICmp(llvm::CmpInst::ICMP_NE, LHS, Zero, "cmp");
> +    RHS = Builder.CreateICmp(llvm::CmpInst::ICMP_NE, RHS, Zero, "cmp");
> +    LHS = Builder.CreateSExt(LHS, Zero->getType(), "sext");
> +    RHS = Builder.CreateSExt(RHS, Zero->getType(), "sext");
> +    return Builder.CreateAnd(LHS, RHS);

What about and'ing the results of the two icmp instructions, and sext'ing
that, i.e.:

LHS = Builder.CreateICmp(llvm::CmpInst::ICMP_NE, LHS, Zero, "cmp");
RHS = Builder.CreateICmp(llvm::CmpInst::ICMP_NE, RHS, Zero, "cmp");
Value *LAnd = Builder.CreateAnd(LHS, RHS);
return Builder.CreateSExt(LAnd, Zero->getType(), "sext");

>  Value *ScalarExprEmitter::VisitBinLOr(const BinaryOperator *E) {
> +  
> +  // If this is vector-typed, just emit two vcmp nodes and a vector and.
> +  if (E->getType()->isVectorType()) {
> +    Value *LHS = Visit(E->getLHS());
> +    Value *RHS = Visit(E->getRHS());
> +    Value *Zero = llvm::ConstantAggregateZero::get(LHS->getType());
> +    LHS = Builder.CreateICmp(llvm::CmpInst::ICMP_NE, LHS, Zero, "cmp");
> +    RHS = Builder.CreateICmp(llvm::CmpInst::ICMP_NE, RHS, Zero, "cmp");
> +    LHS = Builder.CreateSExt(LHS, Zero->getType(), "sext");
> +    RHS = Builder.CreateSExt(RHS, Zero->getType(), "sext");
> +    return Builder.CreateOr(LHS, RHS);

Same here.

Thanks,
-- 
Peter