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