lildmh marked an inline comment as done.
lildmh added inline comments.
================
Comment at: lib/Sema/SemaLookup.cpp:1074
+ if (NameKind == LookupOMPMapperName) {
+ // Skip out-of-scope declarations.
----------------
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?
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
