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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: lib/Sema/SemaLookup.cpp:1074
 
+  if (NameKind == LookupOMPMapperName) {
+    // Skip out-of-scope declarations.
----------------
lildmh wrote:
> 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?
No, in your case we don't. According to the C/C++ rules, if the variable is redeclared, the latest declaration is used. The same rule must be applied to the mapper (according to the standard "The visibility and accessibility of this declaration are the same as those of a variable declared at the same point in the program.")
I think that the code should fail in this case, because you would the error message in case of the variables declarations with the same names in those scopes.


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

https://reviews.llvm.org/D58074