[PATCH] D71179: [OpenMP][WIP] Initial support for `begin/end declare variant`

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D71179#1776487 <https://reviews.llvm.org/D71179#1776487>, @hfinkel wrote:

> In D71179#1776467 <https://reviews.llvm.org/D71179#1776467>, @ABataev wrote:
>
> > In D71179#1776457 <https://reviews.llvm.org/D71179#1776457>, @jdoerfert wrote:
> >
> > > > You're doing absolutely the same thing as the original declare variant implementation.
> > >
> > > I don't think so but if you do why do you oppose this approach?
> > >
> > > > And I don't think it would be correct to add them as multiversiin variants to the original function.
> > >
> > > Why wouldn't it be correct to pick the version through the overload resolution instead of the code generation?
> > >  How this could work is already described in the TODO (CodeGenModule.cpp):
> > >
> > >   // TODO: We should introduce function aliases for `omp declare variant`
> > >   //       directives such that we can treat them through the same overload
> > >   //       resolution scheme (via multi versioning) as `omp begin declare
> > >   //       variant` functions. For an `omp declare variant(VARIANT) ...`
> > >   //       that is attached to a BASE function we would create a global alias
> > >   //       VARIANT = BASE which will participate in the multi version overload
> > >   //       resolution. If picked, here is no need to emit them explicitly.
> > >   
> > >
> > >
> > >
> > >  ---
> > >
> > > I still haven't understood why we cannot/should not reuse the existing multi-version support and instead duplicate the logic in some custom scheme.
> > >  We have this patch that shows how we can reuse the logic in Clang. It works on a per-call basis, so it will work for all context selector (incl. construct).
> > >  If you think there is something conceptually not working, I'd like to hear about it. However, just saying "it wouldn't be correct" is not sufficient. You need to provide details about the situation, what you think would not work, and why.
> >
> >
> > I explayned already: declare variant cannot be represented as mutiversion functiin, for example.
>
>
> @ABataev, can you please elaborate? It's not obvious to me that we cannot handle the existing declare variant with the same scheme (as @jdoerfert highlighted above). In general, I believe it's preferable to have one generic scheme and use it to handle all cases as opposed to continuing to use a more-limited scheme in addition to the generic scheme.


Eaine already. Current version of declare variant cannot be represented as multiversiin functions, because it is not. We have a function that is the alias to other functions with different names. They just are not multiversion functions by definition.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179