[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D71241#1777661 <https://reviews.llvm.org/D71241#1777661>, @jdoerfert wrote:

> In D71241#1776798 <https://reviews.llvm.org/D71241#1776798>, @ABataev wrote:
>
> > You're merging different functions as multiversiin  variants. I don't think this right to overcomplicate the semantics of multiversion functions just because you want to do it.
>
>
> I am actually not doing that here.


You do this when tries to resolve the overloading though it is absolutely not required. You can easily implement it at the codegen phase (it is implemented already, actually). Because you don't need to resolve the overloads, it is resolved already by sema. hat you need to do is to select the correct version of the function and that's it. If you have global traits only, you emit alias. If you have local traits (like construct), you use the address of the best variant function directly. And no need to worry about templates, overloading resolution etc. Plus handling for the corner cases and future changes.

In your solution, you're actually not using mutiversioning at all, you use just one feature from the multiversioning - handling of multiple definitions of the same function. Nothing else. I'm saying that it is better to modify slightly the codegen because there you have to deal with the C-like constrcuts, where you don't need to worry about most of the problematic c++ features. But you insist on moving of all this stuff to Sema and overcomplicate the things.

> What over complication do you mean exactly? Especially because this patch does not touch multi-version functions at all I'm confused by your comment.

Handling of templates, for example. Plus, mixing different functions (with different names). You have it when you try to resolve overloadings though, actually, we don't need to do it, we can easily do this at the codegen.

Also, check how -ast-print works with your solution. It returns different result than expected because you're transform the code too early. It is incorrect behavior.



================
Comment at: clang/lib/Sema/SemaOverload.cpp:9725
+
+  // TODO: Handle template instantiation
+  Best->Function = cast<FunctionDecl>(
----------------
jdoerfert wrote:
> ABataev wrote:
> > Implement all todos and check it with the size of the code where you just need to iterate through all the va4iants and call the existing functions to emit their aliases.
> We do not emit aliases at all with this approach. Emitting aliases does not work for the generic case, e.g., the construct selector trait.
Not directly. I know that it won't work for construct, for construct we'll need a little bit different approach but it is not very hard to implement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71241