[PATCH] D91980: [OpenMP] Add initial support for `omp [begin/end] assumes`

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added a comment.

Why don't yo want to try to implement the scheme similar to the declare target?



================
Comment at: clang/include/clang/Sema/Sema.h:10333
+  /// Check if there is an active global `omp begin assumes` directive.
+  bool isInOpenMPAssumeScope() { return !OMPAssumeScoped.empty(); }
+
----------------
`const` member function


================
Comment at: clang/include/clang/Sema/Sema.h:10336
+  /// Check if there is an active global `omp assumes` directive.
+  bool hasGlobalOpenMPAssumes() { return !OMPAssumeGlobal.empty(); }
+
----------------
`const` member function


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1531
+
+  auto SkipBraces = [&](llvm::StringRef Spelling, bool IssueNote) {
+    BalancedDelimiterTracker T(*this, tok::l_paren,
----------------
I think you will need just capture `this` by value here


================
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},
----------------
Why not express it as clauses?


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1560
+  auto GetAllowedClauseStartValues = [&]() {
+    std::string s;
+    for (const AssumptionClauseMappingInfo &ACMI : AssumptionClauseMappings)
----------------
`S`


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1569
+    SourceLocation StartLoc = Tok.getLocation();
+    int idx = -1;
+    if (Tok.isAnyIdentifier()) {
----------------
`Idx`


================
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);
----------------
ABataev wrote:
> `Idx`
Better to outline it into function/lambda


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3210
+
+    SmallVector<DeclContext *, 8> DeclContexts;
+    DeclContexts.push_back(CurContext);
----------------
What are you trying to do here? Scan all the decls in the current context and mark all the functions with the assumption attributes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91980