[PATCH] D51200: Introduce per-callsite inline intrinsics

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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