[PATCH] D58074: [OpenMP 5.0] Parsing/sema support for map clause with mapper modifier

[Lingda Li via Phabricator via cfe-commits]

lildmh marked 2 inline comments as done.
lildmh added inline comments.


================
Comment at: include/clang/AST/OpenMPClause.h:4236
+  static OMPMapClause *
+  Create(const ASTContext &C, SourceLocation StartLoc, SourceLocation LParenLoc,
+         SourceLocation EndLoc, ArrayRef<Expr *> Vars,
----------------
ABataev wrote:
> lildmh wrote:
> > ABataev wrote:
> > > The function has more than 10 parameters, it is not too good. Would be good to pack some of them into some kind of structure.
> > I can define a structure `OMPMapClauseModifier` within `OMPMapClause` to include all modifier related info. What do you think?
> Yes, it would be good
Then I'll do that. Thanks


================
Comment at: lib/Sema/SemaLookup.cpp:1074
 
+  if (NameKind == LookupOMPMapperName) {
+    // Skip out-of-scope declarations.
----------------
ABataev wrote:
> lildmh wrote:
> > ABataev wrote:
> > > Why do we need special processing of the mapper here?
> > The declare mapper lookup needs to find all `OMPDeclareMapperDecl`s with the same name (the same as declare reduction lookup), and thus it goes through all scopes from the innermost to the outtermost.
> > 
> > Then it looks up a parent scope S (e.g., the outter {} in the following example), all `OMPDeclareMapperDecl`s declared in the children scopes of S will appear in the range between `IdResolver.begin(Name)` and `IdResolver.end()`. Also, the `declare mapper(id: struct S s)` will appear before `omp declare mapper(id: struct SS ss)`. This can cause the lookup function to ignore later `omp declare mapper(id: struct SS ss)` declared in the outter scope. As a result, we may not find the corrent mapper.
> > 
> > ```
> > {
> >   #pragma omp declare mapper(id: struct S s) (s.a)
> >   {
> >     #pragma omp declare mapper(id: struct SS ss) (ss.b)
> >     ...
> >   }
> > }
> > ```
> > 
> > To fix this problem, the purpose of this part of code is to ignore all `OMPDeclareMapperDecl`s in inner scopes, which are already found in previous calls to `LookupParsedName()` from `buildUserDefinedMapperRef`.
> > 
> > I also found that the declare reduction lookup has the same problem. I'll push out a similar fix for the declare reduction later.
> I don't think this is the correct behavior. For user-defined reductions, the standard says "This match is done by means of a name lookup in the base language." It means we should use the lookup rules of C/C++. For mapper, seems to me, we also should completely rely on the visibility/accessibility rules used by C/C++.
Again, thanks for your quick response!

```
{
  #pragma omp declare mapper(id: struct S s) (s.a)
  {
    #pragma omp declare mapper(id: struct SS ss) (ss.b)
    struct S kk;
    #pragma omp target map(mapper(id), tofrom: kk)
    {}
  }
}
```

If I don't add this code, the above code will report error that it cannnot find a mapper with name `id` for type `struct S`, because it can only find the second mapper for type `struct SS`. This doesn't look like correct behavior to me.

I think we are still following the lookup rules of C/C++ with this fix. It's because for declare mapper and reduction, we call `LookupParsedName` multiple times on different scopes. In other cases, `LookupParsedName` is always called once. Because of this difference, I think this fix is appropriate. What is your thought?


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

https://reviews.llvm.org/D58074