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

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert added inline comments.


================
Comment at: clang/lib/Sema/SemaLambda.cpp:999-1002
+  // OpenMP lambdas might get assumumption attributes.
+  if (LangOpts.OpenMP)
+    ActOnFinishedFunctionDefinitionInOpenMPAssumeScope(Method);
+
----------------
ABataev wrote:
> Are there any other function-like constructs that also should have markings? Coroutines maybe? 
Hm, can we postpone coroutines for now?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3203
+  auto *AA = AssumptionAttr::Create(Context, llvm::join(Assumptions, ","), Loc);
+  // Disable assumes in OpenMP simd mode.
+  if (DKind == llvm::omp::Directive::OMPD_begin_assumes) {
----------------
ABataev wrote:
> How this comment relates to the code?
Leftover.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3207
+  } else {
+    assert(DKind == llvm::omp::Directive::OMPD_assumes && "");
+    OMPAssumeGlobal.push_back(AA);
----------------
ABataev wrote:
> Add a message in for the assertion
Good catch!


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:3217
+    while (Ctx->getParent())
+      Ctx = Ctx->getParent();
+    DeclContexts.push_back(Ctx);
----------------
ABataev wrote:
> Maybe, better to use `getLexicalParent`? `getParent` returns semantic parent, while `getLexicalParent` - the lexical parent. Do you need to mark the declarations in the lexical context or in the semantic context?
I go to the top, should not matter. I want all declarations so I start at the top most scope, then traverse everything. I go with lexical now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91980