lildmh marked 11 inline comments as done.
lildmh added a comment.
Hi Alexey,
Thanks very much for your quick review!
For the codegen, currently I'm thinking to do it within declare target codegen
functions. So there is not a very clear interface to do mapper codegen (also,
there is not a clear interface to do map clause codegen now), and that's why I
didn't have a stub left in this patch. Please see other detailed responses
inline.
================
Comment at: include/clang/AST/OpenMPClause.h:4236
+ static OMPMapClause *
+ Create(const ASTContext &C, SourceLocation StartLoc, SourceLocation
LParenLoc,
+ SourceLocation EndLoc, ArrayRef<Expr *> Vars,
----------------
ABataev wrote:
> The function has more than 10 parameters, it is not too good. Would be good
> to pack some of them into some kind of structure.
I can define a structure `OMPMapClauseModifier` within `OMPMapClause` to
include all modifier related info. What do you think?
================
Comment at: lib/Sema/SemaLookup.cpp:1074
+ if (NameKind == LookupOMPMapperName) {
+ // Skip out-of-scope declarations.
----------------
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.
================
Comment at: lib/Sema/SemaLookup.cpp:1793
continue;
+ else if (NameKind == LookupOMPMapperName) {
+ // Skip out-of-scope declarations.
----------------
ABataev wrote:
> Again, what's so special in mapper lookup?
The same as above. This is to fix C lookup, and the above is for C++
================
Comment at: lib/Serialization/ASTReader.cpp:12312
+ UDMappers.reserve(NumVars);
+ for (unsigned i = 0; i < NumVars; ++i)
+ UDMappers.push_back(Record.readExpr());
----------------
ABataev wrote:
> `i`->`I`
I fixed it. Lower case `i` is also used in other loops in this function. Would
you like me to fix them as well?
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
