[PATCH] D71241: [OpenMP][WIP] Use overload centric declare variants

Tue Feb 11 11:28:38 PST 2020

ABataev added a comment.

In D71241#1870218 <https://reviews.llvm.org/D71241#1870218>, @rsmith wrote:

> In D71241#1788003 <https://reviews.llvm.org/D71241#1788003>, @jdoerfert wrote:
>
> > In D71241#1787652 <https://reviews.llvm.org/D71241#1787652>, @hfinkel wrote:
> >
> > > In D71241#1787571 <https://reviews.llvm.org/D71241#1787571>, @ABataev wrote:
> > >
> > > > In D71241#1787265 <https://reviews.llvm.org/D71241#1787265>, @hfinkel wrote:
> > > >
> > > > > In D71241#1786959 <https://reviews.llvm.org/D71241#1786959>, @jdoerfert wrote:
> > > > >
> > > > > > In D71241#1786530 <https://reviews.llvm.org/D71241#1786530>, @ABataev wrote:
> > > > > >
> > > > > > > Most probably, we can use this solution without adding a new expression. `DeclRefExpr` class can contain 2 decls: FoundDecl and the Decl being used. You can use FoundDecl to point to the original function and used decl to point to the function being called in this context. But at first, we need to be sure that we can handle all corner cases correctly.
> > > > > >
> > > > > >
> > > > > > What new expression are you talking about?
> > > > >
> > > > >
> > > > > To be clear, I believe he's talking about the new expression that I proposed we add in order to represent this kind of call. If that's not needed, and we can use the FoundDecl/Decl pair for that purpose, that should also be considered.
> > > > >
> > > > > > This solution already does point to both declarations, as shown here: https://reviews.llvm.org/D71241#1782504
> > > >
> > > >
> > > > Exactly. We need to check if the `MemberRefExpr` can do this too to handle member functions correctly.
> > > >  And need to wait for opinion from Richard Smith about the design. We need to be sure that it won't break compatibility with C/C++ in some corner cases. Design bugs are very hard to solve and I want to be sure that we don't miss anything. And we provide full compatibility with both C and C++.
> > >
> >
>
>
> I've read through some of the relevant parts of the OpenMP 5.0 specification (but not the 5.1 specification), and it looks like this is the same kind of language-specific function resolution that we do in C++: name lookup finds one declaration, which we then statically resolve to a different declaration. As with the C++ case, it seems reasonable and useful to me to represent the statically-selected callee in the AST as the chosen declaration in the `DeclRefExpr` -- that will be the most useful thing for tooling, static analysis, and so on.
>
> However, that seems to lose information in some cases. Consider this:
>
>   void f(int) {}
>  
>   template<typename T> void g(T) {}
>  
>   #pragma omp declare variant(f) match(implementation = {vendor(llvm)})
>   template<> void g(int) {}
>  
>   void h() { g(0); }
>
>
> Here, `h()` calls `f(int)`. The approach in this patch will form a `DeclRefExpr` whose `FoundDecl` is the `FunctionTemplateDecl` `g<T>`, and whose resolved declaration is `f(int)`, but that has no reference to `g<int>` (where the `declare variant` appears). That seems like it could be annoying for some tooling uses to deal with; there's no real way to get back to `g<int>` without redoing template argument deduction or similar.
>
> One possibility to improve the representation would be to replace the existing `NamedDecl*` storage for `FoundDecl`s with a `PointerUnion<NamedDecl*, OpenMPFoundVariantDecl*>`, where a `OpenMPFoundVariantDecl` is an `ASTContext`-allocated struct listing the original found declaration and the function with the `declare variant` pragma.

Hi Richard, thanks for your answer. I agree that this is the best option.

> 
> 
>>> We do need to be careful here. For cases with FoundDecl != Decl, I think that the typo-correction cases look like they probably work, but there are a few places where we make semantic decisions based on the mismatch, such as:
>>> 
>>> In SemaTemplate.cpp below line 512, we have (this is in C++03-specific code):
>>> 
>>>   } else if (!Found.isSuppressingDiagnostics()) {
>>>     //   - if the name found is a class template, it must refer to the same
>>>     //     entity as the one found in the class of the object expression,
>>>     //     otherwise the program is ill-formed.
>>>     if (!Found.isSingleResult() ||
>>>         getAsTemplateNameDecl(Found.getFoundDecl())->getCanonicalDecl() !=
>>>             OuterTemplate->getCanonicalDecl()) {
>>>       Diag(Found.getNameLoc(),
>>>            diag::ext_nested_name_member_ref_lookup_ambiguous)
> 
> This case is only concerned with type templates, so we don't need to worry about it.
> 
>>> and in SemaExpr.cpp near line 2783, we have:
>>> 
>>>   // If we actually found the member through a using declaration, cast
>>>   // down to the using declaration's type.
>>>   //
>>>   // Pointer equality is fine here because only one declaration of a
>>>   // class ever has member declarations.
>>>   if (FoundDecl->getDeclContext() != Member->getDeclContext()) {
>>>     assert(isa<UsingShadowDecl>(FoundDecl));
>>>     QualType URecordType = Context.getTypeDeclType(
>>>                            cast<CXXRecordDecl>(FoundDecl->getDeclContext()));
>> 
>> Could you specify what behavior you expect, or what the test casees would look like?
> 
> This code is handling a particular case of class member access in C++: if you name a member of class Base via a using-declaration in class Derived, we convert first to class Derived and then to class Base, and this is important for the case where (for example) Base is an ambiguous base class. Consider:
> 
>   struct A {
>       void f() {}
>       int n;
>   };
>   
>   struct B : A {
>   #pragma omp declare variant(A::f) match(implementation = {vendor(llvm)})
>       void g() {}
>   };
>   
>   struct C : A, B {};
>   
>   void h(C *p) { p->g(); }
> 
> 
> What I think should happen here (per the OpenMP rules, applied to this case in the most natural way they seem to be applicable) is that we first convert `p` to `B*` (the `this` type of `B::g`), and then convert it to `A*` (the `this` type of the selected variant) -- just like for the source language call `p->A::f()`. That's actually exactly what will happen if you make `B::g` be the found declaration of the member access and `A::f` be the resolved declaration, as this patch does. So I don't think we need changes there, assuming the case above works with this patch -- it seems to crash in code generation without this patch.
> 
> This is also, I think, a fairly decisive argument for representing the variant selection in the AST rather than deferring it to CodeGen -- we want to form the implicit conversion of the object parameter to `A*` in `Sema`.

> Incidentally, if you make `A` a virtual base class of `B`, we produce what seems to be an incorrect and confusing diagnostic:
> 
>   <source>:7:29: error: conversion from pointer to member of class 'A' to pointer to member of class 'B' via virtual base 'A' is not allowed
> 
> 
> Finally, to address the question about what AST fidelity means in this case: we certainly want the AST to represent the program as written. But that's not all: we want the AST to also represent the semantics of the program in a reasonably useful form. For a `DeclRefExpr`, it's more useful to directly point to the statically-chosen declaration than to expect the users of `DeclRefExpr` to find it for themselves, especially since `DeclRefExpr` already has a mechanism to track the syntactic form (the found declaration) separately from the semantically selected declaration.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71241