rsmith added a comment. > Support for constructor calls (`CXXTemporaryExpr`) should also be possible, > but is not the part of this patch.
(You should handle the base class (`CXXConstructExpr`) that describes the semantics, rather than the derived class (`CXXTemporaryObjectExpr`) that describes a particular syntactic form for said semantics.) Have you considered whether the builtin should apply to `new` expressions? (There are potentially three different top-level calls implied by a `new` expression -- an `operator new`, a constructor, and an `operator delete` -- so it's not completely obvious what effect this would have there.) And likewise for `delete`. ================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8236 +def err_argument_to_inline_intrinsic_builtin_call : Error< + "argument to %0 must not be a builtin call">; +def err_argument_to_inline_intrinsic_pdotr_call : Error< ---------------- kuhar wrote: > Quuxplusone wrote: > > I'd expect to be able to write > > > > __builtin_always_inline(sqrt(x)) > > __builtin_no_inline(sqrt(x)) > > > > without caring whether `sqrt` was a real function or just a macro around > > `__builtin_sqrt`. How important is it that calls to builtin functions be > > errors, instead of just being ignored for this purpose? > Very good point. Initially I thought this should be an error, but I think > that since compiler can reason about intrinsics anyway, it makes sense to > treat inline intrinsics as NoOps in this case. I think it would be surprising if `__builtin_no_inline(sqrt(x))` would use a target-specific `sqrt` instruction rather than making a library call. But given that these builtins are best-effort anyway, maybe it's acceptable. ================ Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8238 +def err_argument_to_inline_intrinsic_pdtor_call : Error< + "argument to %0 must not be a pseudo-destructor call">; +def err_argument_to_inline_intrinsic_block_call : Error< ---------------- Nit: we generally use "cannot" rather than "must not" in diagnostics. ================ Comment at: lib/CodeGen/CGBuiltin.cpp:3008-3013 + if (const CXXOperatorCallExpr *OperatorCall = + dyn_cast<CXXOperatorCallExpr>(Arg)) + if (const CXXMethodDecl *MD = + dyn_cast_or_null<CXXMethodDecl>(OperatorCall->getCalleeDecl())) + return EmitCXXOperatorMemberCallExpr(OperatorCall, MD, ReturnValue, + CIK); ---------------- Nit: please add braces around the outer `if`. ================ Comment at: lib/Sema/SemaChecking.cpp:338-345 + const auto CallStmtClass = Call->getStmtClass(); + if (CallStmtClass != Stmt::CallExprClass && + CallStmtClass != Stmt::CXXMemberCallExprClass && + CallStmtClass != Stmt::CXXOperatorCallExprClass) { + S.Diag(BuiltinLoc, diag::err_argument_to_inline_intrinsic_not_call) + << Builtin << Call->getSourceRange(); + return true; ---------------- Use `isa<CallExpr>` rather than checking the exact class here; you should accept statements derived from these kinds too (eg, `UserDefinedLiteralExpr`). (You should explicitly disallow `CUDAKernelCallExpr`, though, since the builtins don't make any sense for a host -> device call.) ================ Comment at: lib/Sema/SemaChecking.cpp:349 + const Decl *TargetDecl = CE->getCalleeDecl(); + if (const auto *FD = dyn_cast_or_null<FunctionDecl>(TargetDecl)) + if (FD->getBuiltinID()) { ---------------- Nit: braces here. ================ Comment at: lib/Sema/SemaChecking.cpp:356-360 + if (CE->getCallee()->getType()->isBlockPointerType()) { + S.Diag(BuiltinLoc, diag::err_argument_to_inline_intrinsic_block_call) + << Builtin << Call->getSourceRange(); + return true; + } ---------------- Is this a necessary restriction? I would expect calls to blocks to be inlineable when the callee can be statically determined. https://reviews.llvm.org/D51200 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits