ABataev added a comment. In D91980#2417049 <https://reviews.llvm.org/D91980#2417049>, @jdoerfert wrote:
> In D91980#2416950 <https://reviews.llvm.org/D91980#2416950>, @ABataev wrote: > >> Why don't yo want to try to implement the scheme similar to the declare >> target? > > Because it is not clear that the standard even says that right now. Also, > what is the user expectation here. > The scheme now is conservative but consistent. I'd prefer to use something > like that first before we clarify edge cases. I don't think that it will affect the implementation significantly. Still, OpenMP standard must preserve visibility etc. rules of C/C++, so the changes should not be critical (I think). ================ Comment at: clang/lib/Parse/ParseOpenMP.cpp:1531 + + auto SkipBraces = [&](llvm::StringRef Spelling, bool IssueNote) { + BalancedDelimiterTracker T(*this, tok::l_paren, ---------------- jdoerfert wrote: > ABataev wrote: > > I think you will need just capture `this` by value here > I've never done that. Could you explain why? Is it just an "optimization"? A > simple test doesn't really show a difference https://godbolt.org/z/bfr47f Yes, this is just the optimization.It is not for the current implementation but for possible future changes. There is no difference for `this` but might be difference if you'll need to capture something else. ================ Comment at: clang/lib/Parse/ParseOpenMP.cpp:1550-1555 + {"absent", false, true, false}, + {"contains", false, true, false}, + {"holds", false, false, true}, + {"no_openmp", false, false, false}, + {"no_openmp_routines", false, false, false}, + {"no_parallelism", false, false, false}, ---------------- jdoerfert wrote: > ABataev wrote: > > Why not express it as clauses? > Various reasons actually: > > - It is a lot of boilerplate code, if we would auto-generate them from the > tablegen definition the situation might be different. > - There seems to be no immediate benefit. > - We want to be in-sync with the generic assumption handling not develop two > systems for the same thing. > Just it is always to have and to maintain just one design rather than 2 or more. But it is up to you. ================ Comment at: clang/lib/Parse/ParseOpenMP.cpp:1569-1593 + int idx = -1; + if (Tok.isAnyIdentifier()) { + II = Tok.getIdentifierInfo(); + llvm::StringSwitch<int> SS(II->getName()); + for (const AssumptionClauseMappingInfo &ACMI : AssumptionClauseMappings) { + if (ACMI.StartsWith) + SS.StartsWith(ACMI.Identifier, ++idx); ---------------- jdoerfert wrote: > ABataev wrote: > > ABataev wrote: > > > `Idx` > > Better to outline it into function/lambda > Which part? Assuming you mean the 9 line conditional above, I'm not > convinced. Even if I do outline it, 4 lines will have to stay here and I > don't see any reuse. The part that searches over the table. ================ Comment at: clang/lib/Sema/SemaOpenMP.cpp:3210 + + SmallVector<DeclContext *, 8> DeclContexts; + DeclContexts.push_back(CurContext); ---------------- jdoerfert wrote: > ABataev wrote: > > What are you trying to do here? Scan all the decls in the current context > > and mark all the functions with the assumption attributes? > Pretty much, yeah. Find all function declarations and add the attribute. I'll > add a comment. Does it mean that for this code ``` void foo(); #pragma omp assume ... void bar(); #pragma omp end assume ``` `foo()` must have the same assumptions like `bar()`? It looks to me, currently we'll end up with the same attributes for both functions. But I'm not sure that this is correct to mark the function outside of the `assumes` region with the same attributes just like the function inside. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91980/new/ https://reviews.llvm.org/D91980 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits