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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D71179#1775834 <https://reviews.llvm.org/D71179#1775834>, @jdoerfert wrote:

> >> This is neither true, nor relevant. It is not true because OpenMP 5.0 declare variant is so broken it cannot be used for what it was intended for. That means people (as for example we for math) will inevitably use begin/end declare variant.
> > 
> > I rather doubt that it is so much broken. The fact, that you need some new construct to express some functionality does not mean that the previous one is incorrect. It is incomplete, maybe. But not broken.
>
> Broken in the sense that we (in the OpenMP accelerator subcommittee) don't think it can be used for what we envisioned it initially. It can be used for certain things though.
>
> > And even for begin/end stuff, multiversioning is only required for construct traits, for all other traits we can reuse the existing implementation.
>
> Again, this is not the case. begin/end *always* caused multiple definitions with the same name. Even if we ignore that for a second, why should we not use the powerful infrastructure we have (=multi-versioning) that supports `construct` traits and not use it for the other traits? Or asked differently, why should we have a second codegen rewriting scheme?


Not always. If we see that the context selector does not match, we can skip everything between begin/end. It means exactly what I said - multiversioning is needed only for `construct` because all other traits can be easily resolved at the compile time. Generally speaking, there are 2 kinds of traits - global traits (like `vendor`, `kind`, `isa`, etc.), which can be resolved completely statically and do not need multiversioning, and local traits, like `construct`, which depend on the OpenMP directives and require something similar to the multiversioning.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179