[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/Sema/Sema.h:11083
 
+  bool CheckLastPrivateForMappedDirectives(ArrayRef<OMPClause *> Clauses);
+
----------------
`checkLastPrivateForMappedDirectives`


================
Comment at: clang/lib/AST/StmtPrinter.cpp:745-746
+  OpenMPDirectiveKind MappedDirective = Node->getMappedDirective();
+  if (MappedDirective != llvm::omp::OMPD_unknown &&
+      MappedDirective == llvm::omp::OMPD_loop) {
+    Indent() << "#pragma omp loop bind(thread)";
----------------
Just `MappedDirective == llvm::omp::OMPD_loop`


================
Comment at: clang/lib/AST/StmtPrinter.cpp:746-748
+      MappedDirective == llvm::omp::OMPD_loop) {
+    Indent() << "#pragma omp loop bind(thread)";
+  } else
----------------
Remove braces


================
Comment at: clang/lib/AST/StmtPrinter.cpp:765-766
+  OpenMPDirectiveKind MappedDirective = Node->getMappedDirective();
+  if (MappedDirective != llvm::omp::OMPD_unknown &&
+      MappedDirective == llvm::omp::OMPD_loop) {
+    Indent() << "#pragma omp loop bind(parallel)";
----------------
Just `MappedDirective == llvm::omp::OMPD_loop`


================
Comment at: clang/lib/AST/StmtPrinter.cpp:766-768
+      MappedDirective == llvm::omp::OMPD_loop) {
+    Indent() << "#pragma omp loop bind(parallel)";
+  } else
----------------
Remove braces


================
Comment at: clang/lib/AST/StmtPrinter.cpp:1005-1006
+  OpenMPDirectiveKind MappedDirective = Node->getMappedDirective();
+  if (MappedDirective != llvm::omp::OMPD_unknown &&
+      MappedDirective == llvm::omp::OMPD_loop) {
+    Indent() << "#pragma omp loop bind(teams)";
----------------
Just `MappedDirective == llvm::omp::OMPD_loop`


================
Comment at: clang/lib/AST/StmtPrinter.cpp:1006-1008
+      MappedDirective == llvm::omp::OMPD_loop) {
+    Indent() << "#pragma omp loop bind(teams)";
+  } else
----------------
Remove braces


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:343
+  /// This may also be used in a similar way for other constructs.
+  OpenMPDirectiveKind MappedDirective = OMPD_unknown;
+
----------------
I assume it shall be a member of SharingMapType, otherwise there might be problems with handling of enclosed constructs.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:646
+  void setCurrentDirective(OpenMPDirectiveKind NewDK) {
+    SharingMapTy *Top = (SharingMapTy *)getTopOfStackOrNull();
+    assert(Top != NULL &&
----------------
Why need a cast here?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:647-648
+    SharingMapTy *Top = (SharingMapTy *)getTopOfStackOrNull();
+    assert(Top != NULL &&
+           "Before calling setCurrentDirective Top of Stack not to be NULL.");
+    // Store the old into MappedDirective & assign argument NewDK to Directive.
----------------
use `nullptr` or just `assert(Top &&`


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:6140-6201
+  llvm::SmallVector<OMPClause *, 8> ClausesWithoutBind;
+  bool UseClausesWithoutBind = false;
+
+  // Restricting to "#pragma omp loop bind"
+  if (Kind == OMPD_loop) {
+    if (BindKind == OMPC_BIND_unknown) {
+      // Setting the enclosing teams or parallel construct for the loop
----------------
Could you outline this new code as a separate function?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:10419
   setFunctionHasBranchProtectedScope();
-  return OMPSimdDirective::Create(Context, StartLoc, EndLoc, NestedLoopCount,
-                                  Clauses, AStmt, B);
+  OMPSimdDirective *tmp = OMPSimdDirective::Create(
+      Context, StartLoc, EndLoc, NestedLoopCount, Clauses, AStmt, B);
----------------
tmp is bad name. Give better name and follow LLVM coding style recommendations.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:10461
   setFunctionHasBranchProtectedScope();
-  return OMPForDirective::Create(
+  OMPForDirective *tmp = OMPForDirective::Create(
       Context, StartLoc, EndLoc, NestedLoopCount, Clauses, AStmt, B,
----------------
Same


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:10474
 
+  /* Check for syntax of lastprivate */
+  if (DSAStack->getMappedDirective() == OMPD_loop) {
----------------
Use c++ style of comments


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:10475-10476
+  /* Check for syntax of lastprivate */
+  if (DSAStack->getMappedDirective() == OMPD_loop) {
+    if (checkGenericLoopLastprivate(*this, Clauses, OMPD_loop, DSAStack))
+      return StmtError();
----------------
`if (DSAStack->getMappedDirective() == OMPD_loop && checkGenericLoopLastprivate(*this, Clauses, OMPD_loop, DSAStack))`. Avoid structured complexity.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:13993
+
+  /* Check for syntax of lastprivate */
+  if (DSAStack->getMappedDirective() == OMPD_loop) {
----------------
Same, c++ 


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:13994-13995
+  /* Check for syntax of lastprivate */
+  if (DSAStack->getMappedDirective() == OMPD_loop) {
+    if (checkGenericLoopLastprivate(*this, Clauses, OMPD_loop, DSAStack))
+      return false;
----------------
Same, avoid complexity


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:13994-13998
+  if (DSAStack->getMappedDirective() == OMPD_loop) {
+    if (checkGenericLoopLastprivate(*this, Clauses, OMPD_loop, DSAStack))
+      return false;
+  }
+  return true;
----------------
ABataev wrote:
> Same, avoid complexity
`return DSAStack->getMappedDirective() == OMPD_loop && checkGenericLoopLastprivate(*this, Clauses, OMPD_loop, DSAStack);


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

https://reviews.llvm.org/D144634