rsmith added a comment.

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.

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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D71241: [... Richard Smith - zygoloid via Phabricator via cfe-commits

Reply via email to