[PATCH] D51200: Introduce per-callsite inline intrinsics

[Jakub Kuderski via Phabricator via cfe-commits]

kuhar 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<
----------------
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.


================
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<
----------------
Quuxplusone wrote:
> 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?
I thought about it an initially disliked the idea of of silently accepting code that cannot be inlined (because there's no function call).

This case is more general, because you can have the same problem with operator calls.
Consider a function template that does `__builtin_always_inline(t + t)`. It makes sense to inline it for some custom type that implements the + operator, but it could be also used when t is e.g. an int. 

Given that I the intrinsics already works on the best-effort basis (and doesn't not strictly guarantee that inlining/no inlining will happen; e.g., indirect calls), and that in the future I also want to introduce a third intrinsic for transitively inlining everything, I think it makes sense to treat it as as NoOp in generic context.

When the context is not generic, maybe it still would make sense to emit an error/warning? I don't like the idea of being able to write `__builtin_always_inline(2 + 3)` and the compiler not complaining at all...  What do you think?


================
Comment at: include/clang/CodeGen/CGFunctionInfo.h:713
+          CanQualType resultType, ArrayRef<CanQualType> argTypes,
+          CallInlineKind inlineKind = CallInlineKind::DefaultInline) {
     ID.AddInteger(info.getCC());
----------------
Quuxplusone wrote:
> 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.
Good suggestion, thanks!


================
Comment at: test/SemaCXX/inline-builtins.cpp:12
+  void operator++();
+  friend S operator+(const S &, const S &);
+};
----------------
Quuxplusone wrote:
> 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.
It should apply to the outermost call, e.g.:
`__builtin_always_inline(A(B(C())))` -- applies to `A(...)`
`A(__builtin_always_inline(B(C())))` --  applies to `B(...)`
`__builtin_always_inline(a.foo().bar())` -- applies to `.bar()`
`__builtin_always_inline(a.foo()).bar()` -- applies to `.foo()`

I added extra testcases for this. Ultimately, I believe that these intrinsic deserve a solid documentation on the clang www for builtins.


================
Comment at: test/SemaCXX/inline-builtins.cpp:12
+  void operator++();
+  friend S operator+(const S &, const S &);
+};
----------------
kuhar wrote:
> Quuxplusone wrote:
> > 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.
> It should apply to the outermost call, e.g.:
> `__builtin_always_inline(A(B(C())))` -- applies to `A(...)`
> `A(__builtin_always_inline(B(C())))` --  applies to `B(...)`
> `__builtin_always_inline(a.foo().bar())` -- applies to `.bar()`
> `__builtin_always_inline(a.foo()).bar()` -- applies to `.foo()`
> 
> I added extra testcases for this. Ultimately, I believe that these intrinsic deserve a solid documentation on the clang www for builtins.
I also plan to have a third inline intrinsic: `__builtin_flatten_inline` that will (try to) transitively inline everything in parentheses.


================
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}}
+
----------------
Quuxplusone wrote:
> Really good you thought of this case! But shouldn't this *not* be an error? `1_s` is an operator call.
This is simply not implemented yet. I added a TODO.
(From what I see, UDL don't look like operators on the AST level.) 


Repository:
  rC Clang

https://reviews.llvm.org/D51200