[PATCH] D99893: [WIP] Replace std::forward & std::move by cast expressions during Sema

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

rsmith added a comment.

In D99893#2669441 <https://reviews.llvm.org/D99893#2669441>, @rjmccall wrote:

> I think a pattern-match in IRGen is probably a much more reasonable alternative if the goal is primarily to avoid the code-generation overheads at -O0 / optimization costs at -O<n>.  You'd still have template instantiation overheads; I don't know how significant those really are.

I agree, this kind of special-casing really goes against the spirit of Clang's AST model, and such special-casing seems to fit better in IR generation than in the AST representation. The approach in this patch is going to require additional complexity in all AST traversals that want to treat function calls uniformly, and will break all existing AST matchers looking for `std::move` / `std::forward` calls.

Perhaps we could instead treat `std::move` and `std::forward` as builtin functions? We could then skip instantiating the definition given that we already have a builtin definition, and IR generation is already set up to perform custom code generation for built-in functions. That should minimize the amount of special-casing and non-uniform AST representation we need here, and closely matches how we model other library functions that have well-known semantic effects. Note that the "builtin function" approach won't allow us to avoid performing overload resolution, but I don't know how important that is for what you're trying to achieve here -- and in any case, skipping overload resolution seems fraught with problems given that there is another (algorithm) overload of `std::move` and that both `std::move` and (especially) `std::forward` will reject some calls in overload resolution.

Treating a namespace-`std` function template as a builtin isn't entirely novel; we already do this for MSVC's `std::_GetExceptionInfo` (though we don't actually handle that properly: we're missing the "namespace `std`" check, at least). Treating the builtin definition as overriding an inline library definition might be novel, though that doesn't seem like a huge problem.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99893/new/

https://reviews.llvm.org/D99893