lildmh marked 2 inline comments as done. lildmh added inline comments.
================ 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: > 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 Then I'll do that. Thanks ================ Comment at: lib/Sema/SemaLookup.cpp:1074 + if (NameKind == LookupOMPMapperName) { + // Skip out-of-scope declarations. ---------------- 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? 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