[PATCH] D153556: [OPENMP52] Initial support for doacross clause.

[Jennifer Yu via Phabricator via cfe-commits]

jyu2 added a comment.

Thanks Alexey for the review.



================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:4415-4439
+  } else if (Kind == OMPC_doacross) {
+    // Handle dependence type for the doacross clause.
+    ColonProtectionRAIIObject ColonRAII(*this);
+    Data.ExtraModifier = getOpenMPSimpleClauseType(
+        Kind, Tok.is(tok::identifier) ? PP.getSpelling(Tok) : "",
+        getLangOpts());
+    Data.ExtraModifierLoc = Tok.getLocation();
----------------
ABataev wrote:
> Can it be unified with depenbd clause parsing? (Maybe in a separate template function)
I don't really has an idea on how to combine this two with template function.  Since depend clause in ordered is deprecated in 52, and will be removed, should we leave as this?


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:91
       llvm::SmallVector<std::pair<Expr *, OverloadedOperatorKind>, 4>;
-  using DoacrossDependMapTy =
-      llvm::DenseMap<OMPDependClause *, OperatorOffsetTy>;
+  using DoacrossDependMapTy = llvm::DenseMap<OMPClause *, OperatorOffsetTy>;
   /// Kind of the declaration used in the uses_allocators clauses.
----------------
ABataev wrote:
> DoacrossClauseMapTy? Since it is intended to handle also doacross clauses.
Changed.  Thanks.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:11299-11344
       if (DC->getDependencyKind() == OMPC_DEPEND_source) {
         if (DependSourceClause) {
           Diag(C->getBeginLoc(), diag::err_omp_more_one_clause)
               << getOpenMPDirectiveName(OMPD_ordered)
               << getOpenMPClauseName(OMPC_depend) << 2;
           ErrorFound = true;
         } else {
----------------
ABataev wrote:
> Try to avoid copy-paste. Maybe introduce templated function?
Not sure how to do this part.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:11359-11371
+  } else if ((DependFound || DoacrossFound) && (TC || SC)) {
+    SourceLocation Loc =
+        DependFound ? DependFound->getBeginLoc() : DoacrossFound->getBeginLoc();
+    Diag(Loc, diag::err_omp_depend_clause_thread_simd)
+        << getOpenMPClauseName(DependFound ? OMPC_depend : OMPC_doacross)
         << getOpenMPClauseName(TC ? TC->getClauseKind() : SC->getClauseKind());
     ErrorFound = true;
----------------
ABataev wrote:
> Same, try to avoid copy-paste
The code is for both Depend and Doacross with is really try to avoid copy-paste.


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:24002
+
+OMPClause *Sema::ActOnOpenMPDoacrossClause(
+    OpenMPDoacrossClauseModifier DepType, SourceLocation DepLoc,
----------------
ABataev wrote:
> Same, if possible try to unify handling with the depend clause, if possible
ActOnOpenMPDoacrossClauseCommon is added which called in 
ActOnOpenMPDependClause
and
ActOnOpenMPDoacrossClause



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

https://reviews.llvm.org/D153556