ABataev added a comment. In D71241#1777972 <https://reviews.llvm.org/D71241#1777972>, @jdoerfert wrote:
> 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. How did you measure it? I have a completely different opinion. Also, tried to reproduce the problem locally, could not reproduce. It seems to me, the output of the test misses several important things. You can check it yourself. `tgt_target_teams()` call uses `@.offload_maptypes` global var but it is not defined. > > >>> 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. Without templates etc. Early resolution of the variant function leads to problems with AST at least. Plus, as I said, problems with handling complex features of C++. > > >> 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. You're completely wrong here! We shall keep the original AST. This is used in several tools and you significantly modify the user code. I consider it a real issue here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71241/new/ https://reviews.llvm.org/D71241 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
