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

Alexey Bataev via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sun Dec 8 19:51:42 PST 2019


ABataev added a comment.

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

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


They do this because they have several function definitions with the same name. In our case, we have several different functions with different names and for us no need to worry about overloading resolution, the compiler will do everything for us.

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





More information about the cfe-commits mailing list