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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked an inline comment as done.
jdoerfert added a comment.

In D71241#1777709 <https://reviews.llvm.org/D71241#1777709>, @ABataev wrote:

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


There is no evidence that this is more complicated. By all measures, this is less complicated (see also below). It is also actually doing the right thing when it comes to code emission. Take https://godbolt.org/z/sJiP3B for example. The calls are wrong and the definition of base is missing.

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

Why is any of this complicated to you? The logic to do the overloading is 15 lines long and most of it is to determine the best of all versions. What about that is more complicated than having multiple patch points during code generation in which we try to modify existing IR but sometimes have to delay it and hope it'll work at the end.

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

This is debatable. AST print does not print the input but the AST, thus what is correct wrt. OpenMP declare variant is nowhere defined but by us.
Arguably, having it print the actually called function and not the base function is preferable. Thus, the new way is actually more informative.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71241