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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
