[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:
> > > > 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.
> > > Hi Alexey,
> > > 
> > > Thanks for your quick response! I don't think it's redeclared in this case, because a mapper has two properties: name and type, just as declare reduction. Only when both name and type are the same, it should be considered as redeclaration.
> > > 
> > > If we consider the above example as redeclaration of a mapper, then we can always just call `LookupParsedName` once to find the most relevant mapper/reductor. Then why do you need to search reductors in a while loop in `buildDeclareReductionRef`?
> > > 
> > > Also, the following example will be correct using the original lookup functions, without my fix:
> > > 
> > > ```
> > > {
> > >   #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 we consider the second mapper as a redeclaration of the first one, this should fail as well. What do you think?
> > The standard clearly states that "The visibility and accessibility of this declaration are the same as those of a variable declared at the same point in the program.". For variables (btw, they also have 2 attributes - name and type) the inner declaration makes the outer one not visible. The same rule should be applied to the mapper.
> > Instead of this case, I'd suggest to implement the argument-dependent lookup for the declarations with the same name but with the different types, declared in the same scope.
> > Your second example should not produce the error as the mappers are declared in the same scope.
> Thanks for your patience! I think I got your point about the visibility and accessibility rule. Now I'm confused about the implementation of declare reduction lookup. Could you explain why you use a `while` loop to call `LookupParsedName` in `buildDeclareReductionRef`? I thought I understood your intention when I read your code, but I'm confused now.
> 
> For argument dependent lookup, I thought it is for functions. Declare reduction can be considered as a function, but I don't think mappers should be considered as functions. What do you think?
It just scans through the scopes until it finds at least one UDR with the specified name, nothing else. It does not try to find all the UDRs with the given name in all scopes.
As for the ADL, mapper, in this case, is similar to UDR because you need to find the mapper with the required name and type.


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

https://reviews.llvm.org/D58074