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:
> > > > > 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?
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
