rsmith added inline comments. ================ Comment at: include/clang/AST/Expr.h:3709 @@ -3708,3 +3708,3 @@ VAArgExpr(SourceLocation BLoc, Expr* e, TypeSourceInfo *TInfo, - SourceLocation RPLoc, QualType t) + SourceLocation RPLoc, QualType t, bool IsMS = false) : Expr(VAArgExprClass, t, VK_RValue, OK_Ordinary, ---------------- Is there a reason to add a default argument for this? It looks like all calls provide an argument.
================ Comment at: include/clang/AST/Expr.h:3720-3722 @@ -3719,4 +3719,5 @@ - /// \brief Create an empty __builtin_va_arg expression. - explicit VAArgExpr(EmptyShell Empty) : Expr(VAArgExprClass, Empty) { } + /// Create an empty __builtin_va_arg expression. + explicit VAArgExpr(EmptyShell Empty, bool IsMS = false) + : Expr(VAArgExprClass, Empty), Val(0), TInfo(0, IsMS) { } ---------------- The `IsMS` flag you added here is never used. ================ Comment at: include/clang/AST/Expr.h:3728-3734 @@ -3726,5 +3727,9 @@ - TypeSourceInfo *getWrittenTypeInfo() const { return TInfo; } - void setWrittenTypeInfo(TypeSourceInfo *TI) { TInfo = TI; } + /// Returns whether this is really a Win64 ABI va_arg expression. + bool isMicrosoftABI() const { return TInfo.getInt(); } + void setIsMicrosoftABI(bool IsMS) { TInfo.setInt(IsMS); } + + TypeSourceInfo *getWrittenTypeInfo() const { return TInfo.getPointer(); } + void setWrittenTypeInfo(TypeSourceInfo *TI) { TInfo.setPointer(TI); } SourceLocation getBuiltinLoc() const { return BuiltinLoc; } ---------------- Probably because its second argument is a type, and we couldn't preserve that and maintain AST fidelity if we represented it as a `CallExpr`. ================ Comment at: lib/CodeGen/CGExprScalar.cpp:3387-3392 @@ -3386,4 +3386,8 @@ - llvm::Value *ArgValue = CGF.EmitVAListRef(VE->getSubExpr()); - llvm::Value *ArgPtr = CGF.EmitVAArg(ArgValue, VE->getType()); + llvm::Value *ArgValue = VE->isMicrosoftABI() ? + CGF.EmitMSVAListRef(VE->getSubExpr()) : + CGF.EmitVAListRef(VE->getSubExpr()); + llvm::Value *ArgPtr = VE->isMicrosoftABI() ? + CGF.EmitMSVAArg(ArgValue, VE->getType()) : + CGF.EmitVAArg(ArgValue, VE->getType()); llvm::Type *ArgTy = ConvertType(VE->getType()); ---------------- Do you really need this dispatch? `CodeGenFunction` dispatches this to the function's ABI info class, which should do the right thing already. ================ Comment at: lib/Sema/SemaChecking.cpp:1038-1040 @@ -1037,1 +1037,5 @@ + case X86::BI__builtin_ms_va_start: + if (Context.getTargetInfo().getTriple().getArch() != llvm::Triple::x86_64) + return false; + return SemaBuiltinVAStart(TheCall); case X86::BI_mm_prefetch: i = 1; l = 0; u = 3; break; ---------------- Can we do better than this? I think it'd be better to check that `__builtin_ms_va_start` only occurs in a function using the MS ABI's variadic calling convention, and `__builtin_va_start` only occurs in a function using the normal variadic C calling convention. http://reviews.llvm.org/D1623 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits