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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D71179#1774469 <https://reviews.llvm.org/D71179#1774469>, @ABataev wrote:

> In D71179#1774448 <https://reviews.llvm.org/D71179#1774448>, @jdoerfert wrote:
>
> > In D71179#1774444 <https://reviews.llvm.org/D71179#1774444>, @ABataev wrote:
> >
> > > I read the spec and don't think that we need all this complex stuff for the implementation. Yiu need judt to check at the codegen phase if the function must be emitted or not. We don't even need to move context checksnfrom codegen, because with the current semantics all the checkscan be safely performed at the codegen phase.
> >
> >
> > For better or worse we need this and it is actually a natural reuse of the multi-versioning code. We need this because:
> >
> > 1. For the begin/end version we cannot even parse anything in a context that does not match at encounter time, e.g. the `kind(fpga)` context in `clang/test/AST/ast-dump-openmp-begin-declare-variant.c`.
>
>
> Ok, here we can check the context and just skip everything between begin/end pragmas just like if something like #ifdef...#endif is seen.


Agreed.

>> 2. For the 5.0 version we cannot use the `replaceAllUses` approach currently implemented in `tryEmitDeclareVariant` as soon as we have the construct context selector trait. That means we will have to resolve the call target earlier anyway.
> 
> I thought about this. Here we need to use a little bit different method, but again everything can be reolved at the codegen phase, no need to resolve it at parsing/sema.

I doubt we can, yet alone want to do (basically) overload resolution during codegen.
Depending on what function we resolve to, we get different instantiations which require everything from semantic analysis to run on that code again, right?
Could you elaborate why we should not do all the overload resolution at the same time and with the same mechanism that is already present? I mean, SemaOverload deals with multi-versioning already.

> Plus, this is completely different problem and must be solved in the different patch.

As I noted in the very beginning of the commit message, this is not supposed to be a commited like this but split into multiple patches. Let's not mix discussions here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71179