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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

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. Even if, as you noted, 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.



================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1531
+
+  auto SkipBraces = [&](llvm::StringRef Spelling, bool IssueNote) {
+    BalancedDelimiterTracker T(*this, tok::l_paren,
----------------
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


================
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},
----------------
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.



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


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3210
+
+    SmallVector<DeclContext *, 8> DeclContexts;
+    DeclContexts.push_back(CurContext);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91980