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

[Alexey Bataev via Phabricator via Openmp-commits]

ABataev added inline comments.


================
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:
> > 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.
> > The part that searches over the table.
> 
> I don't follow. Where is a search? The part that builds a string switch? line 1569 - 1579?
Yes.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3218
+      DeclContext *DC = DeclContexts.pop_back_val();
+      for (auto *SubDC : DC->decls()) {
+        if (auto *CTD = dyn_cast<ClassTemplateDecl>(SubDC)) {
----------------
I would also add a check that the decl is valid here before checking it and applying the attributes.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3225
+        }
+        if (isa<CXXRecordDecl>(SubDC) || isa<NamespaceDecl>(SubDC)) {
+          DeclContexts.push_back(cast<DeclContext>(SubDC));
----------------
```
if (auto *SubCtx = dyn_cast<DeclContext>(SubDC)) {
  DeclContexts.push_back(cast<DeclContext>(SubCtx));
 continue;
}
```
?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3210
+
+    SmallVector<DeclContext *, 8> DeclContexts;
+    DeclContexts.push_back(CurContext);
----------------
jdoerfert wrote:
> ABataev wrote:
> > 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.
> Your example mixes the global `omp assumes` with the scoped `begin/end omp assumes`. The former has the effect to be added to prior declarations, the latter does not have this effect. This is how it is supposed to work:
> ```
> void foo();  // no assumption
>  #pragma omp begin assumes ext_XYZ
> void bar();  // XYZ assumption
>  #pragma omp end assumes
> ```
> ```
> void foo();  // XYZ assumption
>  #pragma omp assumes ext_XYZ
> void bar();  // XYZ assumption
> ```
> 
> 
> 
Ah, I see. `omp assumes` looks weird. :( It may lead to conflicts
PS. What about lambdas? Could add a test that lambdas get annotations too?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91980