[cfe-commits] r51113 - in /cfe/trunk: include/clang/AST/Builtins.def include/clang/AST/Expr.h include/clang/AST/StmtNodes.def include/clang/Basic/DiagnosticKinds.def lib/AST/Expr.cpp lib/AST/StmtPrinter.cpp lib/CodeGen/CGExprScalar.cpp lib/Sema/Sema.h lib/Sema/SemaChecking.cpp lib/Sema/SemaExpr.cpp test/CodeGen/builtinshufflevector.c

[Chris Lattner]

On May 14, 2008, at 12:38 PM, Eli Friedman wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=51113&view=rev
> Log:
> Implementation of __builtin_shufflevector, a portable builtin  
> capable of
> expressing the full flexibility of the LLVM shufflevector instruction.
> The expected immediate usage is in *mmintrin.h, so that they don't
> depend on the mess of gcc-inherited (and not completely implemented)
> shuffle builtins.

Wow, this is great work Eli!


>
> +  int getShuffleMaskIdx(ASTContext &Ctx, unsigned N) {
> +    llvm::APSInt Result(32);
> +    bool result = getExpr(N+2)->isIntegerConstantExpr(Result, Ctx);
> +    assert(result && "Must be integer constant");
> +    return Result.getZExtValue();
> +  }

This should return 'unsigned'.  Also, please assert that N is in range.

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
> +++ cfe/trunk/lib/Sema/SemaChecking.cpp Wed May 14 14:38:39 2008
> @@ -30,7 +30,7 @@
>
> /// CheckFunctionCall - Check a direct function call for various  
> correctness
> /// and safety properties not strictly enforced by the C type system.
> -bool
> +Action::ExprResult
> Sema::CheckFunctionCall(FunctionDecl *FDecl, CallExpr *TheCall) {

Instead of having to delete TheCall on exits, how about using an  
OwningPointer here?

> @@ -200,6 +213,79 @@
>   return false;
> }
>
> +/// SemaBuiltinShuffleVector - Handle __builtin_shufflevector.
> +// This is declared to take (...), so we have to check everything.
> +Action::ExprResult Sema::SemaBuiltinShuffleVector(CallExpr  
> *TheCall) {

Likewise.
>
> +  if (TheCall->getNumArgs() < 3)
> +    return Diag(TheCall->getLocEnd(),  
> diag::err_typecheck_call_too_few_args,
> +                TheCall->getSourceRange());

This forgot to delete TheCall.  Owning pointer is nice for this :)

> +  QualType FAType = TheCall->getArg(0)->getType();
> +  QualType SAType = TheCall->getArg(1)->getType();
> +
> +  if (!FAType->isVectorType() || !SAType->isVectorType()) {
> +    Diag(TheCall->getLocStart(), diag::err_shufflevector_non_vector,
> +         SourceRange(TheCall->getArg(0)->getLocStart(),
> +                     TheCall->getArg(1)->getLocEnd()));
> +    delete TheCall;
> +    return true;
> +  }
> +
> +  if (TheCall->getArg(0)- 
> >getType().getCanonicalType().getUnqualifiedType() !=

Please use SAType and FAType to shrinkify this.

> +  llvm::SmallVector<Expr*, 32> exprs;
> +
> +  for (unsigned i = 0; i < TheCall->getNumArgs(); i++) {
> +    exprs.push_back(TheCall->getArg(i));
> +    TheCall->setArg(i, 0);
> +  }

One somewhat subtle thing here is that shufflevector requires its  
arguments to be i32 in the LLVM IR level.  Right now, I suspect the  
code generator will crash if you pass in 1LL as an element value.

I think it would be reasonable to have the code generator use  
isConstantExpr instead of 'EmitScalarExpr' in the code generator.   
Alternatively, you could have the AST have implicit casts to int.  The  
problem with that is that int may not be i32... I'll let you decide  
what is best.

Thanks again for tackling this, it is very useful!

-Chris