[PATCH] D58074: [OpenMP 5.0] Parsing/sema support for map clause with mapper modifier

[Lingda Li via Phabricator via cfe-commits]

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