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

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D58074#1394993 <https://reviews.llvm.org/D58074#1394993>, @lildmh wrote:

> 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.


This till must be handled somehow in codegen, better to ignore for now, I think.



================
Comment at: include/clang/AST/OpenMPClause.h:4236
+  static OMPMapClause *
+  Create(const ASTContext &C, SourceLocation StartLoc, SourceLocation LParenLoc,
+         SourceLocation EndLoc, ArrayRef<Expr *> Vars,
----------------
lildmh wrote:
> 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?
Yes, it would be good


================
Comment at: lib/Sema/SemaLookup.cpp:1074
 
+  if (NameKind == LookupOMPMapperName) {
+    // Skip out-of-scope declarations.
----------------
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++.


================
Comment at: lib/Serialization/ASTReader.cpp:12312
+  UDMappers.reserve(NumVars);
+  for (unsigned i = 0; i < NumVars; ++i)
+    UDMappers.push_back(Record.readExpr());
----------------
lildmh wrote:
> 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?
This is the old code that was committed before the rules were defined. Soon they're going again to update the coding standard to allow lowCamel for the variables, but currently, only CamelCase is used for variables.
No, don't do this.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D58074/new/

https://reviews.llvm.org/D58074