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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
