[PATCH] D51200: Introduce per-callsite inline intrinsics

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.


================
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<
----------------
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?


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8238
+def err_argument_to_inline_intrinsic_pdotr_call : Error<
+  "argument to %0 must not be a pseudo-destructor call">;
+def err_argument_to_inline_intrinsic_block_call : Error<
----------------
Spelling: `s/dotr/dtor/`
Semantics: Does this trigger in generic contexts? I feel like I'd want to be able to write

    __builtin_always_inline(p->~T());

without caring whether `T` was a primitive type or not. How important is it that pseudo-destructor calls be errors, instead of just being ignored for this purpose?


================
Comment at: include/clang/CodeGen/CGFunctionInfo.h:713
+          CanQualType resultType, ArrayRef<CanQualType> argTypes,
+          CallInlineKind inlineKind = CallInlineKind::DefaultInline) {
     ID.AddInteger(info.getCC());
----------------
Here and throughout, wouldn't it be more traditional to name the parameter variable `CIK`? (You have sometimes `CIK`, sometimes `inlineKind`, sometimes `InlineKind`.)

It also feels needlessly convoluted to have a value of type CallInlineKind stored in a member named InlineCall (note the reversal of the words), but I'm not sure how that's generally dealt with.


================
Comment at: test/SemaCXX/inline-builtins.cpp:12
+  void operator++();
+  friend S operator+(const S &, const S &);
+};
----------------
This raises a question for me about the semantics of "always inlining" a "call":

    struct A { A(); A(A&&); };
    struct B { B(A); }
    void foo(B);

    __builtin_always_inline(foo(A{}));

Does `__builtin_always_inline` apply to `foo`, `B(A)` (used to create the parameter to `foo`), `A()` (used to create the argument to `A(A&&)`), or all of the above?
You should add a test case something like this, maybe with multiple function arguments.


================
Comment at: test/SemaCXX/inline-builtins.cpp:71
+  S s2 = __builtin_always_inline(1_s); // expected-error {{argument to __builtin_always_inline must be a function, member function, or operator call}}
+  s2 = __builtin_no_inline(1_s);       // expected-error {{argument to __builtin_no_inline must be a function, member function, or operator call}}
+
----------------
Really good you thought of this case! But shouldn't this *not* be an error? `1_s` is an operator call.


Repository:
  rC Clang

https://reviews.llvm.org/D51200