[PATCH] D144634: [Clang][OpenMP] Support for Code Generation of loop bind clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:9792
+  "construct">;
+def error_loop_reduction_clause : Error<
+  "reduction clause not handled with '#pragma omp loop bind(teams)'">;
----------------
err_omp_...


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:6146
+
+      const OpenMPDirectiveKind parentDirective =
+          DSAStack->getParentDirective();
----------------
ParentDirective


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:6167-6168
+      // Spec restriction : bind(teams) and reduction not permitted.
+      if ((BindKind == OMPC_BIND_teams) &&
+          (C->getClauseKind() == llvm::omp::Clause::OMPC_reduction))
+        Diag(SourceLocation(), diag::error_loop_reduction_clause);
----------------
Remove extra parens


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:6173-6175
+      if (C->getClauseKind() != llvm::omp::Clause::OMPC_bind) {
+        ClausesWithoutBind.push_back(C);
+      }
----------------
Remove extra braces


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:6181-6189
+      DSAStack->setCurrentDirective(OMPD_for);
+      break;
+    case OMPC_BIND_teams:
+      Kind = OMPD_distribute;
+      DSAStack->setCurrentDirective(OMPD_distribute);
+      break;
+    case OMPC_BIND_thread:
----------------
ABataev wrote:
> 1. You're overriding the directive kind here and do not restore it then. It may cause the compiler crash.
> 2. I think you need to here to create a new OpenMP region rather than overriding the existing one.
I see why you're doing this now. Would be good to see some semantic tests, like nesting of regions, use of unsupported clauses (e.g. linear on loop bind(teams)), etc.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:6199-6202
+  if (UseClausesWithoutBind) {
+    ClausesWithImplicit.append(ClausesWithoutBind.begin(),
+                               ClausesWithoutBind.end());
+  } else {
----------------
Why need to drop bind clause here?


================
Comment at: clang/test/OpenMP/loop_bind_codegen.cpp:51-56
+void thread_loop2() {
+  #pragma omp loop bind(thread)
+  for (int j = 0 ; j < NNN ; j++) {
+    aaa[j] = j*NNN;
+  }
+}
----------------
koops wrote:
> ABataev wrote:
> > I think it should trigger the assert in setCurrentDirective
> I did not see any crash at this point. Can you please give me other examples where I can see this crash?
Ah, you're overriding the loop directive kind.


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

https://reviews.llvm.org/D144634