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

[Alexey Bataev via Phabricator via cfe-commits]

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